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

2018-02-03 Thread asfgit 
Github user asfgit closed the pull request at:

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


---

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

2018-02-03 Thread kumarvishal09 
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 


---

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

2018-02-03 Thread kumarvishal09 
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 


---

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

2018-02-03 Thread jackylk 
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


---

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

2018-02-03 Thread jackylk 
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`


---

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

2018-02-03 Thread jackylk 
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


---

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

2018-02-03 Thread jackylk 
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


---

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

2018-02-03 Thread jackylk 
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


---

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

2018-02-03 Thread jackylk 
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`


---

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

2018-02-03 Thread jackylk 
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


---

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

2018-01-31 Thread kumarvishal09 
Github user kumarvishal09 commented on a diff in the pull request:

https://github.com/apache/carbondata/pull/1888#discussion_r165013843
  
--- Diff: 
core/src/main/java/org/apache/carbondata/core/constants/CarbonCommonConstants.java
 ---
@@ -1567,6 +1567,14 @@
 
   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 --

Fixed


---

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

2018-01-30 Thread ravipesala 
Github user ravipesala commented on a diff in the pull request:

https://github.com/apache/carbondata/pull/1888#discussion_r164962280
  
--- Diff: 
integration/spark2/src/main/scala/org/apache/spark/sql/hive/CarbonPreAggregateRules.scala
 ---
@@ -461,6 +467,8 @@ case class CarbonPreAggregateQueryRules(sparkSession: 
SparkSession) extends Rule
   childPlan,
   carbonTable,
   agg)
+  
CarbonSession.threadSet(CarbonCommonConstants.SUPPORT_DIRECT_QUERY_ON_DATAMAP,
--- End diff --

Can you avoid calling it multiple times , instead call only once


---

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

2018-01-30 Thread jackylk 
Github user jackylk commented on a diff in the pull request:

https://github.com/apache/carbondata/pull/1888#discussion_r164933909
  
--- Diff: 
integration/spark2/src/main/scala/org/apache/spark/sql/optimizer/CarbonLateDecodeRule.scala
 ---
@@ -87,6 +88,39 @@ class CarbonLateDecodeRule extends Rule[LogicalPlan] 
with PredicateHelper {
 }
   }
 
+  /**
+   * Below method will be used to validate if query is directly fired on 
pre aggregate
+   * data map or not
+   * @param relations all relations from query
+   *
+   */
+  def validateQueryDirectlyOnDataMap(relations: 
Seq[CarbonDecoderRelation]): Unit = {
+var isPreAggDataMapExists = false
+// first check if pre aggregate data map exists or not
+relations.foreach{relation =>
+  if (relation.carbonRelation.carbonTable.isChildDataMap) {
+isPreAggDataMapExists = true
+  }
+}
+val validateQuery = CarbonProperties.getInstance
+  .getProperty(CarbonCommonConstants.VALIDATE_DIRECT_QUERY_ON_DATAMAP, 
"true").toBoolean
+var isThrowException = false
+// if validate query is enabled and relation contains pre aggregate 
data map
+if (validateQuery && isPreAggDataMapExists) {
+  val carbonSessionInfo = ThreadLocalSessionInfo.getCarbonSessionInfo
--- End diff --

Can you expose a put and get interface in CarbonEnv to manipulate the 
SessionParam in CarbonEnv


---

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

2018-01-30 Thread jackylk 
Github user jackylk commented on a diff in the pull request:

https://github.com/apache/carbondata/pull/1888#discussion_r164933498
  
--- Diff: 
core/src/main/java/org/apache/carbondata/core/constants/CarbonCommonConstants.java
 ---
@@ -1567,6 +1567,14 @@
 
   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 --

provide default value for these two newly added property


---

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

2018-01-30 Thread kumarvishal09 
GitHub user kumarvishal09 opened a pull request:

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

[CARBONDATA-2101]Restrict direct query on pre aggregate and timeseries 
datamap

**Restricting direct query on PreAggregate and timeseries data map 
Added Property to run direct query on data map for testing purpose
validate.support.direct.query.on.datamap=true**

 - [ ] Any interfaces changed?
 
 - [ ] Any backward compatibility impacted?
 
 - [ ] Document update required?

 - [ ] Testing done
Added UT Test case 
   
 - [ ] For large changes, please consider breaking it into sub-tasks under 
an umbrella JIRA. 



You can merge this pull request into a Git repository by running:

$ git pull https://github.com/kumarvishal09/incubator-carbondata 
Restrict_DirectQuery_OnDatamap

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/carbondata/pull/1888.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #1888


commit b5aaf49f289394f3800fa5e8b3133ba79ef0d869
Author: Manohar 
Date:   2017-12-20T09:39:45Z

Restrict direct query on pre aggregate and timeseries datamap




---