[GitHub] [carbondata] marchpure opened a new pull request #3839: [CARBONDATA-3898] Support Option 'carbon.enable.querywithmv'

[GitBox]

marchpure opened a new pull request #3839:
URL: https://github.com/apache/carbondata/pull/3839


    ### Why is this PR needed?
   When MV enabled, SQL rewrite takes a lot of time, a new option 'carbon.enable.querywithmv' shall be supported, which can turn off SQL Rewrite when the configured value is false
   
    ### What changes were proposed in this PR?
   Add option 'carbon.enable.querywithmv', then logicplan won't be changed if configured value is false
       
    ### Does this PR introduce any user interface change?
    - No
   
    ### Is any new testcase added?
    - Yes
   
       
   


----------------------------------------------------------------
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:
[hidden email]

[GitBox]

CarbonDataQA1 commented on pull request #3839:
URL: https://github.com/apache/carbondata/pull/3839#issuecomment-657271121


   Build Failed  with Spark 2.4.5, Please check CI http://121.244.95.60:12545/job/ApacheCarbon_PR_Builder_2.4.5/1620/
   


----------------------------------------------------------------
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:
[hidden email]

[GitBox]

CarbonDataQA1 commented on pull request #3839:
URL: https://github.com/apache/carbondata/pull/3839#issuecomment-657271385


   Build Failed  with Spark 2.3.4, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.3/3361/
   


----------------------------------------------------------------
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:
[hidden email]

[GitBox]

kevinjmh commented on a change in pull request #3839:
URL: https://github.com/apache/carbondata/pull/3839#discussion_r453392507



##########
File path: core/src/main/java/org/apache/carbondata/core/util/CarbonProperties.java
##########
@@ -1762,6 +1762,20 @@ public boolean isDistributedPruningEnabled(String dbName, String tableName) {
     return isServerEnabledByUser;
   }
 
+  /**
+   * Check whether the MV is enabled by the user or not.
+   */
+  public boolean isMVEnabled() {
+    String mvEnabled = CarbonProperties.getInstance().getProperty(
+            CarbonCommonConstants.CARBON_ENABLE_QUERYWITHMV);
+    if (mvEnabled == null) {
+      return Boolean.parseBoolean(CarbonCommonConstants.CARBON_ENABLE_QUERYWITHMV_DEFAULT);
+    } else {
+      // return false only if false is set. any other case return true
+      return !mvEnabled.equalsIgnoreCase("false");

Review comment:
       ```suggestion
         return mvEnabled.equalsIgnoreCase("true");
   ```




----------------------------------------------------------------
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:
[hidden email]

[GitBox]

kevinjmh commented on a change in pull request #3839:
URL: https://github.com/apache/carbondata/pull/3839#discussion_r453393117



##########
File path: integration/spark/src/test/scala/org/apache/carbondata/view/MVTest.scala
##########
@@ -63,6 +63,30 @@ class MVTest extends QueryTest with BeforeAndAfterAll {
     sql("drop table source")
   }
 
+  test("test disable mv with carbonproperties and sessionparam") {
+    assert(CarbonProperties.getInstance().isMVEnabled)
+    sql("drop materialized view if exists mv1")
+    sql("drop table if exists source")
+    sql("create table source as select * from fact_table")
+    sql("create materialized view mv1 as select empname, deptname, avg(salary) from source group by empname, deptname")
+
+    CarbonProperties.getInstance().addProperty(CarbonCommonConstants.CARBON_ENABLE_QUERYWITHMV,"false")
+    var df = sql("select empname, avg(salary) from source group by empname")
+    assert(!isTableAppearedInPlan(df.queryExecution.optimizedPlan, "mv1"))
+
+    df = sql("select empname, avg(salary) from source group by empname")
+    CarbonProperties.getInstance().removeProperty(CarbonCommonConstants.CARBON_ENABLE_QUERYWITHMV)
+    assert(isTableAppearedInPlan(df.queryExecution.optimizedPlan, "mv1"))
+
+    sql("set carbon.mv.enable = false")

Review comment:
       not consist with the property name added. And I think this one is better than `carbon.enable.querywithmv`




----------------------------------------------------------------
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:
[hidden email]

[GitBox]

kevinjmh commented on a change in pull request #3839:
URL: https://github.com/apache/carbondata/pull/3839#discussion_r453394545



##########
File path: integration/spark/src/main/scala/org/apache/spark/sql/optimizer/MVRewriteRule.scala
##########
@@ -98,14 +98,8 @@ class MVRewriteRule(session: SparkSession) extends Rule[LogicalPlan] {
     if (!canApply) {
       return logicalPlan
     }
-    val sessionInformation = ThreadLocalSessionInfo.getCarbonSessionInfo
-    if (sessionInformation != null && sessionInformation.getThreadParams != null) {
-      val disableViewRewrite = sessionInformation.getThreadParams.getProperty(
-        CarbonCommonConstants.DISABLE_SQL_REWRITE)
-      if (disableViewRewrite != null &&
-        disableViewRewrite.equalsIgnoreCase("true")) {
-        return logicalPlan
-      }

Review comment:
       please check this changes again, for the case multiple sessions with different setting




----------------------------------------------------------------
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:
[hidden email]

[GitBox]

marchpure commented on a change in pull request #3839:
URL: https://github.com/apache/carbondata/pull/3839#discussion_r453433678



##########
File path: core/src/main/java/org/apache/carbondata/core/util/CarbonProperties.java
##########
@@ -1762,6 +1762,20 @@ public boolean isDistributedPruningEnabled(String dbName, String tableName) {
     return isServerEnabledByUser;
   }
 
+  /**
+   * Check whether the MV is enabled by the user or not.
+   */
+  public boolean isMVEnabled() {
+    String mvEnabled = CarbonProperties.getInstance().getProperty(
+            CarbonCommonConstants.CARBON_ENABLE_QUERYWITHMV);
+    if (mvEnabled == null) {
+      return Boolean.parseBoolean(CarbonCommonConstants.CARBON_ENABLE_QUERYWITHMV_DEFAULT);
+    } else {
+      // return false only if false is set. any other case return true
+      return !mvEnabled.equalsIgnoreCase("false");

Review comment:
       modified

##########
File path: integration/spark/src/test/scala/org/apache/carbondata/view/MVTest.scala
##########
@@ -63,6 +63,30 @@ class MVTest extends QueryTest with BeforeAndAfterAll {
     sql("drop table source")
   }
 
+  test("test disable mv with carbonproperties and sessionparam") {
+    assert(CarbonProperties.getInstance().isMVEnabled)
+    sql("drop materialized view if exists mv1")
+    sql("drop table if exists source")
+    sql("create table source as select * from fact_table")
+    sql("create materialized view mv1 as select empname, deptname, avg(salary) from source group by empname, deptname")
+
+    CarbonProperties.getInstance().addProperty(CarbonCommonConstants.CARBON_ENABLE_QUERYWITHMV,"false")
+    var df = sql("select empname, avg(salary) from source group by empname")
+    assert(!isTableAppearedInPlan(df.queryExecution.optimizedPlan, "mv1"))
+
+    df = sql("select empname, avg(salary) from source group by empname")
+    CarbonProperties.getInstance().removeProperty(CarbonCommonConstants.CARBON_ENABLE_QUERYWITHMV)
+    assert(isTableAppearedInPlan(df.queryExecution.optimizedPlan, "mv1"))
+
+    sql("set carbon.mv.enable = false")

Review comment:
       modified




----------------------------------------------------------------
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:
[hidden email]

[GitBox]

marchpure commented on a change in pull request #3839:
URL: https://github.com/apache/carbondata/pull/3839#discussion_r453433760



##########
File path: integration/spark/src/main/scala/org/apache/spark/sql/optimizer/MVRewriteRule.scala
##########
@@ -98,14 +98,8 @@ class MVRewriteRule(session: SparkSession) extends Rule[LogicalPlan] {
     if (!canApply) {
       return logicalPlan
     }
-    val sessionInformation = ThreadLocalSessionInfo.getCarbonSessionInfo
-    if (sessionInformation != null && sessionInformation.getThreadParams != null) {
-      val disableViewRewrite = sessionInformation.getThreadParams.getProperty(
-        CarbonCommonConstants.DISABLE_SQL_REWRITE)
-      if (disableViewRewrite != null &&
-        disableViewRewrite.equalsIgnoreCase("true")) {
-        return logicalPlan
-      }

Review comment:
       checked, some testcases are added




----------------------------------------------------------------
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:
[hidden email]

[GitBox]

CarbonDataQA1 commented on pull request #3839:
URL: https://github.com/apache/carbondata/pull/3839#issuecomment-657393900


   Build Success with Spark 2.3.4, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.3/3365/
   


----------------------------------------------------------------
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:
[hidden email]

[GitBox]

CarbonDataQA1 commented on pull request #3839:
URL: https://github.com/apache/carbondata/pull/3839#issuecomment-657394159


   Build Failed  with Spark 2.4.5, Please check CI http://121.244.95.60:12545/job/ApacheCarbon_PR_Builder_2.4.5/1624/
   


----------------------------------------------------------------
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:
[hidden email]

[GitBox]

marchpure commented on pull request #3839:
URL: https://github.com/apache/carbondata/pull/3839#issuecomment-657405972


   retest this please


----------------------------------------------------------------
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:
[hidden email]

[GitBox]

CarbonDataQA1 commented on pull request #3839:
URL: https://github.com/apache/carbondata/pull/3839#issuecomment-657487436


   Build Success with Spark 2.4.5, Please check CI http://121.244.95.60:12545/job/ApacheCarbon_PR_Builder_2.4.5/1627/
   


----------------------------------------------------------------
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:
[hidden email]

[GitBox]

CarbonDataQA1 commented on pull request #3839:
URL: https://github.com/apache/carbondata/pull/3839#issuecomment-657487681


   Build Success with Spark 2.3.4, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.3/3368/
   


----------------------------------------------------------------
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:
[hidden email]

[GitBox]

CarbonDataQA1 commented on pull request #3839:
URL: https://github.com/apache/carbondata/pull/3839#issuecomment-657974163


   Build Success with Spark 2.3.4, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.3/3383/
   


----------------------------------------------------------------
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:
[hidden email]

[GitBox]

CarbonDataQA1 commented on pull request #3839:
URL: https://github.com/apache/carbondata/pull/3839#issuecomment-657974400


   Build Success with Spark 2.4.5, Please check CI http://121.244.95.60:12545/job/ApacheCarbon_PR_Builder_2.4.5/1642/
   


----------------------------------------------------------------
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:
[hidden email]

[GitBox]

xubo245 commented on a change in pull request #3839:
URL: https://github.com/apache/carbondata/pull/3839#discussion_r454134062



##########
File path: core/src/main/java/org/apache/carbondata/core/util/CarbonProperties.java
##########
@@ -501,6 +502,25 @@ private void validateCarbonTaskDistribution() {
     }
   }
 
+  private void validateEnableMV() {
+    String isMVEnabled = carbonProperties.getProperty(CarbonCommonConstants.CARBON_ENABLE_MV);
+    if (isMVEnabled == null) {
+      carbonProperties.setProperty(CarbonCommonConstants.CARBON_ENABLE_MV,
+          CarbonCommonConstants.CARBON_ENABLE_MV_DEFAULT);
+      isMVEnabled = carbonProperties.getProperty(CarbonCommonConstants.CARBON_ENABLE_MV);
+    }
+    boolean isValidBooleanValue = CarbonUtil.validateBoolean(isMVEnabled);
+    if (!isValidBooleanValue) {

Review comment:
       suggestion:  merge it with line 507,  null also is invalid value.




----------------------------------------------------------------
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:
[hidden email]

[GitBox]

xubo245 commented on a change in pull request #3839:
URL: https://github.com/apache/carbondata/pull/3839#discussion_r454134453



##########
File path: core/src/main/java/org/apache/carbondata/core/util/CarbonProperties.java
##########
@@ -501,6 +502,25 @@ private void validateCarbonTaskDistribution() {
     }
   }
 
+  private void validateEnableMV() {
+    String isMVEnabled = carbonProperties.getProperty(CarbonCommonConstants.CARBON_ENABLE_MV);
+    if (isMVEnabled == null) {
+      carbonProperties.setProperty(CarbonCommonConstants.CARBON_ENABLE_MV,
+          CarbonCommonConstants.CARBON_ENABLE_MV_DEFAULT);
+      isMVEnabled = carbonProperties.getProperty(CarbonCommonConstants.CARBON_ENABLE_MV);
+    }
+    boolean isValidBooleanValue = CarbonUtil.validateBoolean(isMVEnabled);
+    if (!isValidBooleanValue) {

Review comment:
       CarbonUtil.validateBoolean  include judge null:
   
   ```
     public static boolean validateBoolean(String value) {
       if (null == value) {
         return false;
       } else if (!("false".equalsIgnoreCase(value) || "true".equalsIgnoreCase(value))) {
         return false;
       }
       return true;
     }
   ```




----------------------------------------------------------------
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:
[hidden email]

[GitBox]

xubo245 commented on a change in pull request #3839:
URL: https://github.com/apache/carbondata/pull/3839#discussion_r454134453



##########
File path: core/src/main/java/org/apache/carbondata/core/util/CarbonProperties.java
##########
@@ -501,6 +502,25 @@ private void validateCarbonTaskDistribution() {
     }
   }
 
+  private void validateEnableMV() {
+    String isMVEnabled = carbonProperties.getProperty(CarbonCommonConstants.CARBON_ENABLE_MV);
+    if (isMVEnabled == null) {
+      carbonProperties.setProperty(CarbonCommonConstants.CARBON_ENABLE_MV,
+          CarbonCommonConstants.CARBON_ENABLE_MV_DEFAULT);
+      isMVEnabled = carbonProperties.getProperty(CarbonCommonConstants.CARBON_ENABLE_MV);
+    }
+    boolean isValidBooleanValue = CarbonUtil.validateBoolean(isMVEnabled);
+    if (!isValidBooleanValue) {

Review comment:
       CarbonUtil.validateBoolean  include judge null:
   
   ```
     public static boolean validateBoolean(String value) {
       if (null == value) {
         return false;
       } else if (!("false".equalsIgnoreCase(value) || "true".equalsIgnoreCase(value))) {
         return false;
       }
       return true;
     }
   ```
   
   so  line 507-511 is unnecessary




----------------------------------------------------------------
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:
[hidden email]

[GitBox]

xubo245 commented on a change in pull request #3839:
URL: https://github.com/apache/carbondata/pull/3839#discussion_r454137628



##########
File path: core/src/main/java/org/apache/carbondata/core/constants/CarbonCommonConstants.java
##########
@@ -2433,12 +2441,6 @@ private CarbonCommonConstants() {
    */
   public static final String INDEX_STATUS = "index_status";
 
-  /**

Review comment:
       Are there any user already use this configuration?




----------------------------------------------------------------
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:
[hidden email]

[GitBox]

xubo245 commented on a change in pull request #3839:
URL: https://github.com/apache/carbondata/pull/3839#discussion_r454137683



##########
File path: core/src/main/java/org/apache/carbondata/core/constants/CarbonCommonConstants.java
##########
@@ -2433,12 +2441,6 @@ private CarbonCommonConstants() {
    */
   public static final String INDEX_STATUS = "index_status";
 
-  /**
-   * Materialized view thread context properties
-   */
-  @CarbonProperty
-  public static final String DISABLE_SQL_REWRITE = "disable_sql_rewrite";

Review comment:
       Are there any user already use this configuration?




----------------------------------------------------------------
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:
[hidden email]