[GitHub] carbondata pull request #2335: [WIP] integrate carbonstore mv branch

[qiuchenjian-2]

Github user ravipesala commented on the issue:

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



---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

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



---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

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



---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2335#discussion_r192743275
 
    --- Diff: datamap/mv/core/src/main/scala/org/apache/carbondata/mv/datamap/MVHelper.scala ---
    @@ -373,5 +373,25 @@ object MVHelper {
           case other => other
         }
       }
    +
    +  def rewriteWithMVTable(rewrittenPlan: ModularPlan, rewrite: QueryRewrite): ModularPlan = {
    --- End diff --
   
    please add comment for this func


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2335#discussion_r192744678
 
    --- Diff: datamap/mv/core/src/main/scala/org/apache/carbondata/mv/datamap/MVState.scala ---
    @@ -31,25 +31,25 @@ private[mv] class MVState(summaryDatasetCatalog: SummaryDatasetCatalog) {
       // Note: These are all lazy vals because they depend on each other (e.g. conf) and we
       // want subclasses to override some of the fields. Otherwise, we would get a lot of NPEs.
     
    -  /**
    -   * Modular query plan modularizer
    -   */
    -  lazy val modularizer = SimpleModularizer
    -
    -  /**
    -   * Logical query plan optimizer.
    -   */
    -  lazy val optimizer = BirdcageOptimizer
    -
    -  lazy val matcher = DefaultMatchMaker
    -
    -  lazy val navigator: Navigator = new Navigator(summaryDatasetCatalog, this)
    +//  /**
    +//   * Modular query plan modularizer
    +//   */
    +//  lazy val modularizer = SimpleModularizer
    +//
    +//  /**
    +//   * Logical query plan optimizer.
    +//   */
    --- End diff --
   
    Is it for debugging purpose?


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2335#discussion_r192945440
 
    --- Diff: datamap/mv/core/src/main/scala/org/apache/carbondata/mv/datamap/MVHelper.scala ---
    @@ -373,5 +373,25 @@ object MVHelper {
           case other => other
         }
       }
    +
    +  def rewriteWithMVTable(rewrittenPlan: ModularPlan, rewrite: QueryRewrite): ModularPlan = {
    --- End diff --
   
    ok


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2335#discussion_r192945546
 
    --- Diff: datamap/mv/core/src/main/scala/org/apache/carbondata/mv/datamap/MVState.scala ---
    @@ -31,25 +31,25 @@ private[mv] class MVState(summaryDatasetCatalog: SummaryDatasetCatalog) {
       // Note: These are all lazy vals because they depend on each other (e.g. conf) and we
       // want subclasses to override some of the fields. Otherwise, we would get a lot of NPEs.
     
    -  /**
    -   * Modular query plan modularizer
    -   */
    -  lazy val modularizer = SimpleModularizer
    -
    -  /**
    -   * Logical query plan optimizer.
    -   */
    -  lazy val optimizer = BirdcageOptimizer
    -
    -  lazy val matcher = DefaultMatchMaker
    -
    -  lazy val navigator: Navigator = new Navigator(summaryDatasetCatalog, this)
    +//  /**
    +//   * Modular query plan modularizer
    +//   */
    +//  lazy val modularizer = SimpleModularizer
    +//
    +//  /**
    +//   * Logical query plan optimizer.
    +//   */
    --- End diff --
   
    Not required, removed


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

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



---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

    https://github.com/apache/carbondata/pull/2335
 
    @jackylk Please review and merge


---

[qiuchenjian-2]

Github user eychih commented on the issue:

    https://github.com/apache/carbondata/pull/2335
 
    can you merge changes in chowbranch into master?  code in chowbranch has passed test cases in both old and new test cases and has support for the left outer join view?


---

[qiuchenjian-2]

Github user eychih commented on the issue:

    https://github.com/apache/carbondata/pull/2335
 
    I saw you already integrate some code in chowbranch.  However, in MVAnalyzerRule.scala, there are some issues:
    1. At line 68, you call withMVTable directly.  The issue is, if the method throws exceptions, there is no way to handle them.
    2. where do you replace the factored out MV subplan with the corresponding MV table? (please refer the same file in chowbranch, to see how this get handled)  


---

[qiuchenjian-2]

Github user eychih commented on the issue:

    https://github.com/apache/carbondata/pull/2335
 
    For 2 in the previous comment,  in SummaryDatasetCatalog.scala, I add a method useSummaryDataset(plan: LogicalPlan) to replace a subplan of an MV with the corresponding MV table.



---

[qiuchenjian-2]

Github user eychih commented on the issue:

    https://github.com/apache/carbondata/pull/2335
 
    In addition, for consistency, please use the following way to remove PreAggFunction:
   
        val planToRegister = sparkSession.sql(updatedQuery).queryExecution.analyzed.
          transform {
            case p @ logical.Project(head :: tail, _) if head.isInstanceOf[Alias] && head.name.equals("preAgg") => p.copy(projectList = tail)
            case a @ logical.Aggregate(_, head :: tail, _) if head.isInstanceOf[Alias] && head.name.equals("preAgg") => a.copy(aggregateExpressions = tail)
          }


---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

    https://github.com/apache/carbondata/pull/2335
 
    @eychih I have merged the necessary changes from chowbranch to this PR. And left outer join changes will be taken in a different PR.  Factored out MV subplan to table is handled in `withMVTable` method. I cannot use the chowbranch's way it give issues related to incremental loading of table and sameresult cannot work if the session is changed.And also it has performnace impact if we use the chowbranch's way. Dropping of preAgg function is handled in MVHelper class



---

[qiuchenjian-2]

Github user eychih commented on the issue:

    https://github.com/apache/carbondata/pull/2335
 
    @ravipesala Incremental loading of MV tables should be done separately from query rewrite.  Mixing of query rewrite with incremental loading defeats the purpose of MV.  Also, the method sameresult is independent of sessions.  The method can be applied to logical plans created in different sessions.  Finally, the way used in the chowbranch will have better performance when an MV is cached in memory because Spark replaces the corresponding subplan with an in-memory table first.


---