[GitHub] [carbondata] VenuReddy2103 opened a new pull request #3772: [CARBONDATA-3832]Added block and blocket pruning for the polygon expression processing

[GitBox]

Indhumathi27 commented on a change in pull request #3772:
URL: https://github.com/apache/carbondata/pull/3772#discussion_r440609387



##########
File path: core/src/main/java/org/apache/carbondata/core/scan/filter/FilterUtil.java
##########
@@ -188,6 +189,14 @@ private static FilterExecuter createFilterExecuterTree(
           return new FalseFilterExecutor();
         case ROWLEVEL:
         default:
+          if (filterExpressionResolverTree.getFilterExpression() instanceof UnknownExpression) {
+            FilterExecuter filterExecuter =
+                ((UnknownExpression) filterExpressionResolverTree.getFilterExpression())
+                    .getFilterExecuter(filterExpressionResolverTree, segmentProperties);
+            if (filterExecuter != null) {

Review comment:
       If in case, filterExecuter is null, we need to return RowLevelFilterExecuterImpl right?. If filterExecuter never comes null, can check and remove this check

##########
File path: integration/spark/src/test/scala/org/apache/carbondata/geo/GeoTest.scala
##########
@@ -134,8 +133,31 @@ class GeoTest extends QueryTest with BeforeAndAfterAll with BeforeAndAfterEach {
         Row(116285807, 40084087)))
   }
 
-  test("test polygon query on table partitioned by timevalue column")
-  {
+  test("test block pruning for polygon query") {
+    createTable()
+    sql(s"insert into $table1 select 1575428400000,116285807,40084087")
+    sql(s"insert into $table1 select 1575428400000,116372142,40129503")
+    sql(s"insert into $table1 select 1575428400000,116187332,39979316")
+    sql(s"insert into $table1 select 1575428400000,116337069,39951887")
+    sql(s"insert into $table1 select 1575428400000,116359102,40154684")
+    sql(s"insert into $table1 select 1575428400000,116736367,39970323")
+    sql(s"insert into $table1 select 1575428400000,116362699,39942444")
+    sql(s"insert into $table1 select 1575428400000,116325378,39963129")
+    sql(s"insert into $table1 select 1575428400000,116302895,39930753")
+    sql(s"insert into $table1 select 1575428400000,116288955,39999101")
+    val df = sql(s"select longitude, latitude from $table1 where IN_POLYGON('116.321011 " +
+                 s"40.123503, 116.137676 39.947911, 116.560993 39.935276, 116.321011 40.123503')")
+    assert(df.rdd.getNumPartitions == 6)
+    checkAnswer(df,

Review comment:
       Can you move this checkAnswer to a new method with param as dataframe. Looks like this check is same in all test cases




----------------------------------------------------------------
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]

VenuReddy2103 commented on a change in pull request #3772:
URL: https://github.com/apache/carbondata/pull/3772#discussion_r445475371



##########
File path: integration/spark/src/test/scala/org/apache/carbondata/geo/GeoTest.scala
##########
@@ -134,8 +133,31 @@ class GeoTest extends QueryTest with BeforeAndAfterAll with BeforeAndAfterEach {
         Row(116285807, 40084087)))
   }
 
-  test("test polygon query on table partitioned by timevalue column")
-  {
+  test("test block pruning for polygon query") {
+    createTable()
+    sql(s"insert into $table1 select 1575428400000,116285807,40084087")
+    sql(s"insert into $table1 select 1575428400000,116372142,40129503")
+    sql(s"insert into $table1 select 1575428400000,116187332,39979316")
+    sql(s"insert into $table1 select 1575428400000,116337069,39951887")
+    sql(s"insert into $table1 select 1575428400000,116359102,40154684")
+    sql(s"insert into $table1 select 1575428400000,116736367,39970323")
+    sql(s"insert into $table1 select 1575428400000,116362699,39942444")
+    sql(s"insert into $table1 select 1575428400000,116325378,39963129")
+    sql(s"insert into $table1 select 1575428400000,116302895,39930753")
+    sql(s"insert into $table1 select 1575428400000,116288955,39999101")
+    val df = sql(s"select longitude, latitude from $table1 where IN_POLYGON('116.321011 " +
+                 s"40.123503, 116.137676 39.947911, 116.560993 39.935276, 116.321011 40.123503')")
+    assert(df.rdd.getNumPartitions == 6)
+    checkAnswer(df,

Review comment:
       Ok. Instead of making the Seq of rows with same rows in all testcases, have defined it once and used same in all the testcases.




----------------------------------------------------------------
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]

VenuReddy2103 commented on a change in pull request #3772:
URL: https://github.com/apache/carbondata/pull/3772#discussion_r445481119



##########
File path: core/src/main/java/org/apache/carbondata/core/scan/filter/FilterUtil.java
##########
@@ -188,6 +189,14 @@ private static FilterExecuter createFilterExecuterTree(
           return new FalseFilterExecutor();
         case ROWLEVEL:
         default:
+          if (filterExpressionResolverTree.getFilterExpression() instanceof UnknownExpression) {
+            FilterExecuter filterExecuter =
+                ((UnknownExpression) filterExpressionResolverTree.getFilterExpression())
+                    .getFilterExecuter(filterExpressionResolverTree, segmentProperties);
+            if (filterExecuter != null) {

Review comment:
       I understand what you meant. I think, it is better to create the RowLevelFilterExecuterImpl only at this place. If UnknownExpression do not have any specific implemenation, it will be like a fallback to RowLevelFilterExecuterImpl.
    Also, if we have return RowLevelFilterExecuterImpl from UnknownExpression.getFilterExecuter, we need to pass complexDimensionInfoMap and all overiding methods will have this argument. It is an unused argument as current UnKnown expressions do not support complex dimensions.




----------------------------------------------------------------
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 #3772:
URL: https://github.com/apache/carbondata/pull/3772#issuecomment-649532771


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


----------------------------------------------------------------
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 #3772:
URL: https://github.com/apache/carbondata/pull/3772#issuecomment-649533543


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


----------------------------------------------------------------
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]

kunal642 commented on pull request #3772:
URL: https://github.com/apache/carbondata/pull/3772#issuecomment-655957238


   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 #3772:
URL: https://github.com/apache/carbondata/pull/3772#issuecomment-656030202


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


----------------------------------------------------------------
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 #3772:
URL: https://github.com/apache/carbondata/pull/3772#issuecomment-656031544


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


----------------------------------------------------------------
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]

brijoobopanna commented on pull request #3772:
URL: https://github.com/apache/carbondata/pull/3772#issuecomment-657349617


   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 #3772:
URL: https://github.com/apache/carbondata/pull/3772#issuecomment-657388354


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


----------------------------------------------------------------
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 #3772:
URL: https://github.com/apache/carbondata/pull/3772#issuecomment-657388621


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


----------------------------------------------------------------
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]

kunal642 commented on pull request #3772:
URL: https://github.com/apache/carbondata/pull/3772#issuecomment-657408569


   @VenuReddy2103 Please check the failure


----------------------------------------------------------------
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]

VenuReddy2103 commented on pull request #3772:
URL: https://github.com/apache/carbondata/pull/3772#issuecomment-657437317


   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 #3772:
URL: https://github.com/apache/carbondata/pull/3772#issuecomment-657510101


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


----------------------------------------------------------------
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 #3772:
URL: https://github.com/apache/carbondata/pull/3772#issuecomment-657514136


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


----------------------------------------------------------------
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]

kunal642 commented on pull request #3772:
URL: https://github.com/apache/carbondata/pull/3772#issuecomment-657533884


   LGTM


----------------------------------------------------------------
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]

asfgit closed pull request #3772:
URL: https://github.com/apache/carbondata/pull/3772


   


----------------------------------------------------------------
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]