[GitHub] carbondata pull request #2453: [CARBONDATA-2528][MV] Fixed order by in mv an...

[qiuchenjian-2]

Github user jackylk commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/2453#discussion_r202224158
 
    --- Diff: datamap/mv/core/src/main/scala/org/apache/carbondata/mv/rewrite/SummaryDatasetCatalog.scala ---
    @@ -39,6 +40,12 @@ private[mv] case class SummaryDataset(signature: Option[Signature],
         dataMapSchema: DataMapSchema,
         relation: ModularPlan)
     
    +case class MVPlanWrapper(plan: ModularPlan, dataMapSchema: DataMapSchema) extends ModularPlan {
    --- End diff --
   
    Why is this needed? package visibility problem?


---

[qiuchenjian-2]

Github user ravipesala commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/2453#discussion_r202244060
 
    --- Diff: datamap/mv/core/src/main/scala/org/apache/carbondata/mv/datamap/MVHelper.scala ---
    @@ -147,6 +149,34 @@ object MVHelper {
         }.filter(_.isDefined).map(_.get)
       }
     
    +
    +  /**
    +   * Check if we can do incremental load on the mv table. Some cases like aggregation functions
    +   * which are present inside other expressions like sum(a)+sum(b) cannot be incremental loaded.
    +   */
    +  private def isFullReload(logicalPlan: LogicalPlan): Boolean = {
    +    var isFullReload = false
    +    logicalPlan.transformAllExpressions {
    +      case a: Alias =>
    +        a
    +      case agg: AggregateExpression => agg
    +      case c: Cast =>
    +        isFullReload = c.child.find {
    +          case agg: AggregateExpression => false
    +          case _ => false
    +        }.isDefined || isFullReload
    +        c
    +      case exp: Expression =>
    +        // Check any aggregation function present inside other expression.
    +        isFullReload = exp.find {
    +          case agg: AggregateExpression => true
    --- End diff --
   
    no, it should be true. If the aggregation function present inside any other expression like add(sum(a) + sum(b)) then we should always do full refresh instead of incremental refresh


---

[qiuchenjian-2]

Github user ravipesala commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/2453#discussion_r202244238
 
    --- Diff: datamap/mv/core/src/main/scala/org/apache/carbondata/mv/datamap/MVHelper.scala ---
    @@ -147,6 +149,34 @@ object MVHelper {
         }.filter(_.isDefined).map(_.get)
       }
     
    +
    +  /**
    +   * Check if we can do incremental load on the mv table. Some cases like aggregation functions
    --- End diff --
   
    Yes, it checks whether a logical plan can support full refresh or incremental refresh.


---

[qiuchenjian-2]

Github user ravipesala commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/2453#discussion_r202244323
 
    --- Diff: datamap/mv/core/src/main/scala/org/apache/carbondata/mv/datamap/MVHelper.scala ---
    @@ -179,7 +209,7 @@ object MVHelper {
           case _ => false
         }
     
    -    override def hashCode: Int = exp.hashCode
    +    override def hashCode: Int = 1
    --- End diff --
   
    added


---

[qiuchenjian-2]

Github user ravipesala commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/2453#discussion_r202244376
 
    --- Diff: datamap/mv/core/src/main/scala/org/apache/carbondata/mv/datamap/MVHelper.scala ---
    @@ -290,16 +327,18 @@ object MVHelper {
        * @return Updated modular plan.
        */
       def updateDataMap(subsumer: ModularPlan, rewrite: QueryRewrite): ModularPlan = {
    -    subsumer match {
    +     subsumer match {
    --- End diff --
   
    ok


---

[qiuchenjian-2]

Github user ravipesala commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/2453#discussion_r202244579
 
    --- Diff: datamap/mv/core/src/main/scala/org/apache/carbondata/mv/rewrite/SummaryDatasetCatalog.scala ---
    @@ -39,6 +40,12 @@ private[mv] case class SummaryDataset(signature: Option[Signature],
         dataMapSchema: DataMapSchema,
         relation: ModularPlan)
     
    +case class MVPlanWrapper(plan: ModularPlan, dataMapSchema: DataMapSchema) extends ModularPlan {
    --- End diff --
   
    No, it is to provide datamap schema information along with relation. Added comment as well.


---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

    https://github.com/apache/carbondata/pull/2453
 
    SDV Build Success , Please check CI http://144.76.159.231:8080/job/ApacheSDVTests/5814/



---

[qiuchenjian-2]

Github user brijoobopanna commented on the issue:

    https://github.com/apache/carbondata/pull/2453
 
    retest this please


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2453
 
    Build Failed  with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder1/7099/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2453
 
    Build Success with Spark 2.2.1, Please check CI http://88.99.58.216:8080/job/ApacheCarbonPRBuilder/5875/



---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

    https://github.com/apache/carbondata/pull/2453
 
    retest this please


---

[qiuchenjian-2]

Github user jackylk commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/2453#discussion_r202504966
 
    --- Diff: datamap/mv/core/src/main/scala/org/apache/carbondata/mv/datamap/MVHelper.scala ---
    @@ -147,6 +149,34 @@ object MVHelper {
         }.filter(_.isDefined).map(_.get)
       }
     
    +
    +  /**
    +   * Check if we can do incremental load on the mv table. Some cases like aggregation functions
    +   * which are present inside other expressions like sum(a)+sum(b) cannot be incremental loaded.
    +   */
    +  private def isFullReload(logicalPlan: LogicalPlan): Boolean = {
    +    var isFullReload = false
    +    logicalPlan.transformAllExpressions {
    +      case a: Alias =>
    +        a
    +      case agg: AggregateExpression => agg
    +      case c: Cast =>
    +        isFullReload = c.child.find {
    +          case agg: AggregateExpression => false
    +          case _ => false
    +        }.isDefined || isFullReload
    +        c
    +      case exp: Expression =>
    +        // Check any aggregation function present inside other expression.
    +        isFullReload = exp.find {
    +          case agg: AggregateExpression => true
    --- End diff --
   
    Can you modify comment to tell like `return true if .... `



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2453
 
    Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder1/7179/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2453
 
    Build Success with Spark 2.2.1, Please check CI http://88.99.58.216:8080/job/ApacheCarbonPRBuilder/5955/



---

[qiuchenjian-2]

Github user jackylk commented on the issue:

    https://github.com/apache/carbondata/pull/2453
 
    LGTM


---

[qiuchenjian-2]

Github user asfgit closed the pull request at:

    https://github.com/apache/carbondata/pull/2453


---