[GitHub] carbondata pull request #1888: [CARBONDATA-2101]Restrict direct query on pre...

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

    https://github.com/apache/carbondata/pull/1888
 
    retest sdv please


---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

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



---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/1888
 
    Build Failed with Spark 2.2.1, Please check CI http://88.99.58.216:8080/job/ApacheCarbonPRBuilder/2205/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

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



---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

    https://github.com/apache/carbondata/pull/1888
 
    @kumarvishal09 Please rebase it


---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

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



---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/1888#discussion_r165809259
 
    --- Diff: integration/spark-common-test/src/test/scala/org/apache/carbondata/integration/spark/testsuite/preaggregate/TestPreAggregateTableSelection.scala ---
    @@ -25,9 +25,14 @@ import org.scalatest.BeforeAndAfterAll
     
     import org.apache.carbondata.core.metadata.schema.datamap.DataMapProvider.TIMESERIES
     
    +import org.apache.carbondata.core.constants.CarbonCommonConstants
    +import org.apache.carbondata.core.util.CarbonProperties
    +
     class TestPreAggregateTableSelection extends QueryTest with BeforeAndAfterAll {
     
       override def beforeAll: Unit = {
    +    CarbonProperties.getInstance()
    +      .addProperty(CarbonCommonConstants.VALIDATE_DIRECT_QUERY_ON_DATAMAP, "false")
    --- End diff --
   
    add this property in QueryTest so no need to add in all test suite class


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/1888#discussion_r165809273
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/util/SessionParams.java ---
    @@ -199,6 +199,8 @@ private boolean validateKeyValue(String key, String value) throws InvalidConfigu
               }
             } else if (key.startsWith(CarbonCommonConstants.VALIDATE_CARBON_INPUT_SEGMENTS)) {
               isValid = true;
    +        } else if (key.startsWith(CarbonCommonConstants.SUPPORT_DIRECT_QUERY_ON_DATAMAP)) {
    --- End diff --
   
    I think should use `equals` instead of `startsWith`


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/1888#discussion_r165809300
 
    --- Diff: integration/spark2/src/main/scala/org/apache/spark/sql/execution/command/preaaggregate/PreAggregateUtil.scala ---
    @@ -599,6 +599,8 @@ object PreAggregateUtil {
           CarbonCommonConstants.VALIDATE_CARBON_INPUT_SEGMENTS +
           parentTableIdentifier.database.getOrElse(sparkSession.catalog.currentDatabase) + "." +
           parentTableIdentifier.table, validateSegments.toString)
    +    CarbonSession.threadSet(CarbonCommonConstants.SUPPORT_DIRECT_QUERY_ON_DATAMAP,
    --- End diff --
   
    why this is required


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/1888#discussion_r165809302
 
    --- Diff: integration/spark2/src/main/scala/org/apache/spark/sql/hive/CarbonPreAggregateRules.scala ---
    @@ -259,6 +259,7 @@ case class CarbonPreAggregateQueryRules(sparkSession: SparkSession) extends Rule
        * @return transformed plan
        */
       def transformPreAggQueryPlan(logicalPlan: LogicalPlan): LogicalPlan = {
    +    var isPlanUpdated = false
    --- End diff --
   
    why this is required


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/1888#discussion_r165809341
 
    --- Diff: integration/spark-common-test/src/test/scala/org/apache/carbondata/integration/spark/testsuite/preaggregate/TestPreAggregateTableSelection.scala ---
    @@ -324,6 +329,8 @@ test("test PreAggregate table selection with timeseries and normal together") {
         sql("drop table if exists mainTable")
         sql("drop table if exists mainTable_avg")
         sql("drop table if exists lineitem")
    +    CarbonProperties.getInstance()
    +      .addProperty(CarbonCommonConstants.VALIDATE_DIRECT_QUERY_ON_DATAMAP, "true")
         sql("DROP TABLE IF EXISTS maintabletime")
       }
     
    --- End diff --
   
    add a test case to verify user can not query the child table when CarbonCommonConstants.VALIDATE_DIRECT_QUERY_ON_DATAMAP is false


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/1888#discussion_r165809409
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/constants/CarbonCommonConstants.java ---
    @@ -1585,6 +1585,16 @@
     
       public static final String CARBON_ENABLE_PAGE_LEVEL_READER_IN_COMPACTION_DEFAULT = "true";
     
    +  @CarbonProperty
    +  public static final String SUPPORT_DIRECT_QUERY_ON_DATAMAP =
    +      "carbon.support.direct.query.on.datamap";
    --- End diff --
   
    change the property name to: `carbon.directQueryOnDataMap.enabled`


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/1888#discussion_r165809447
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/constants/CarbonCommonConstants.java ---
    @@ -1590,6 +1590,16 @@
           "carbon.sort.storage.inmemory.size.inmb";
       public static final String IN_MEMORY_STORAGE_FOR_SORTED_DATA_IN_MB_DEFAULT = "512";
     
    +  @CarbonProperty
    +  public static final String SUPPORT_DIRECT_QUERY_ON_DATAMAP =
    +      "carbon.support.direct.query.on.datamap";
    --- End diff --
   
    change the property to carbon.query.directQueryOnDataMap.enabled, naming convention should similar to spark and hadoop


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/1888#discussion_r165810948
 
    --- Diff: integration/spark2/src/main/scala/org/apache/spark/sql/execution/command/preaaggregate/PreAggregateUtil.scala ---
    @@ -599,6 +599,8 @@ object PreAggregateUtil {
           CarbonCommonConstants.VALIDATE_CARBON_INPUT_SEGMENTS +
           parentTableIdentifier.database.getOrElse(sparkSession.catalog.currentDatabase) + "." +
           parentTableIdentifier.table, validateSegments.toString)
    +    CarbonSession.threadSet(CarbonCommonConstants.SUPPORT_DIRECT_QUERY_ON_DATAMAP,
    --- End diff --
   
    After applying pre aggregate rule we need to skip direct query on data map check


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/1888#discussion_r165810950
 
    --- Diff: integration/spark2/src/main/scala/org/apache/spark/sql/hive/CarbonPreAggregateRules.scala ---
    @@ -259,6 +259,7 @@ case class CarbonPreAggregateQueryRules(sparkSession: SparkSession) extends Rule
        * @return transformed plan
        */
       def transformPreAggQueryPlan(logicalPlan: LogicalPlan): LogicalPlan = {
    +    var isPlanUpdated = false
    --- End diff --
   
    After applying pre aggregate rule we need to skip direct query on data map check


---