[GitHub] [carbondata] akashrn5 commented on a change in pull request #4038: [CARBONDATA-4076] Fix MV having Subquery alias used in query projection

[GitBox <gi...@apache.org>]

akashrn5 commented on a change in pull request #4038:
URL: https://github.com/apache/carbondata/pull/4038#discussion_r543346052



##########
File path: integration/spark/src/main/scala/org/apache/spark/sql/optimizer/MVMatcher.scala
##########
@@ -803,6 +836,9 @@ private object SelectSelectNoChildDelta extends MVMatchPattern with PredicateHel
         sel_3q @ modular.Select(_, _, _, _, _, _, _, _, _, _), None)
         if sel_3a.children.forall(_.isInstanceOf[GroupBy]) &&
            sel_3q.children.forall(_.isInstanceOf[GroupBy]) =>
+        LOGGER.debug(

Review comment:
       same as above and please change in all places.

##########
File path: integration/spark/src/main/scala/org/apache/spark/sql/optimizer/MVMatcher.scala
##########
@@ -647,6 +680,10 @@ private object SelectSelectNoChildDelta extends MVMatchPattern with PredicateHel
         ) if sel_1a.children.forall { _.isInstanceOf[modular.LeafNode] } &&
              sel_1q.children.forall { _.isInstanceOf[modular.LeafNode] } =>
 
+        LOGGER.debug(
+          "Applying pattern: {SelectSelectNoChildDelta} for the plan: {" +
+          subsumee.toString().trim + " }. Current Subsumer: {" + subsumer.toString().trim + " }")

Review comment:
       do not use the `+`, since scala class use `s""`

##########
File path: integration/spark/src/main/scala/org/apache/spark/sql/optimizer/MVMatcher.scala
##########
@@ -228,6 +230,34 @@ private abstract class MVMatchPattern extends Logging {
     }
   }
 
+  /**
+   * Compares the output list of subsumer/subsumee with/without alias. In case if, expression
+   * is instance of Alias, then compare it's child expression.
+   */
+  protected def compareOutputList(outputList1: Seq[NamedExpression],

Review comment:
       rename to `subsumerOutputList` and `subsumeeOutputList`

##########
File path: integration/spark/src/main/scala/org/apache/spark/sql/optimizer/MVRewriteRule.scala
##########
@@ -154,6 +158,9 @@ class MVRewriteRule(session: SparkSession) extends Rule[LogicalPlan] {
       case _ =>
         compactSQL
     }
+    LOGGER.debug("Converting from Modular Plan to Logical Plan for query " +

Review comment:
       you can just add here as rewritten query and print query

##########
File path: integration/spark/src/test/scala/org/apache/carbondata/view/rewrite/MVCreateTestCase.scala
##########
@@ -414,6 +414,20 @@ class MVCreateTestCase extends QueryTest with BeforeAndAfterAll {
     sql(s"drop materialized view mv32")
   }
 
+  test("test create materialized view having sub-query alias used in projection") {
+    sql("drop materialized view if exists mv_sub")
+    val subQuery = "select empname, sum(result) sum_ut from " +
+                   "(select empname, utilization result from fact_table1) fact_table1 " +
+                   "group by empname"
+    sql("create materialized view mv_sub as " + subQuery)

Review comment:
       can you add a test where MV is created on `select empname, utilization result from fact_table1` and then actual query uses this as subquery?




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org