[carbondata] branch master updated: [CARBONDATA-3315] Fix for Range Filter failing with two between clauses as children of OR expression

[ra...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

ravipesala pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/carbondata.git


The following commit(s) were added to refs/heads/master by this push:
     new 8328cc7  [CARBONDATA-3315] Fix for Range Filter failing with two between clauses as children of OR expression
8328cc7 is described below

commit 8328cc7f794995bc1be602dc62941e4b93726995
Author: manishnalla1994 <ma...@gmail.com>
AuthorDate: Tue Mar 12 18:58:43 2019 +0530

    [CARBONDATA-3315] Fix for Range Filter failing with two between clauses as children of OR expression
    
    Problem : Range filter with Or and two between clauses as children fails because while evaluating the Or we combine both the sub-trees into one and then evaluate, which is wrong.
    
    Solution : Instead of combining them we have to treat both the children of 'OR' as different expressions and evaluate separately.
    
    This closes #3145
---
 .../scan/expression/RangeExpressionEvaluator.java  | 30 +++++++++++++----
 .../detailquery/RangeFilterTestCase.scala          | 38 ++++++++++++++++++++++
 2 files changed, 61 insertions(+), 7 deletions(-)

diff --git a/core/src/main/java/org/apache/carbondata/core/scan/expression/RangeExpressionEvaluator.java b/core/src/main/java/org/apache/carbondata/core/scan/expression/RangeExpressionEvaluator.java
index c47d5ff..f131c92 100644
--- a/core/src/main/java/org/apache/carbondata/core/scan/expression/RangeExpressionEvaluator.java
+++ b/core/src/main/java/org/apache/carbondata/core/scan/expression/RangeExpressionEvaluator.java
@@ -208,6 +208,13 @@ public class RangeExpressionEvaluator {
     return filterExpressionMap;
   }
 
+  private void evaluateOrExpression(Expression currentNode, Expression orExpChild) {
+    Map<String, List<FilterModificationNode>> filterExpressionMapNew =
+        new HashMap<>(CarbonCommonConstants.DEFAULT_COLLECTION_SIZE);
+    fillExpressionMap(filterExpressionMapNew, orExpChild, currentNode);
+    replaceWithRangeExpression(filterExpressionMapNew);
+    filterExpressionMapNew.clear();
+  }
 
   private void fillExpressionMap(Map<String, List<FilterModificationNode>> filterExpressionMap,
       Expression currentNode, Expression parentNode) {
@@ -222,13 +229,22 @@ public class RangeExpressionEvaluator {
         && eligibleForRangeExpConv(currentNode))) {
       addFilterExpressionMap(filterExpressionMap, currentNode, parentNode);
     }
-
-    for (Expression exp : currentNode.getChildren()) {
-      if (null != exp) {
-        fillExpressionMap(filterExpressionMap, exp, currentNode);
-        if (exp instanceof OrExpression) {
-          replaceWithRangeExpression(filterExpressionMap);
-          filterExpressionMap.clear();
+    // In case of Or Exp we have to evaluate both the subtrees of expression separately
+    // else it will combine the results of both the subtrees into one expression
+    // which wont give us correct result
+    if (currentNode instanceof OrExpression) {
+      Expression leftChild = ((OrExpression) currentNode).left;
+      Expression rightChild = ((OrExpression) currentNode).right;
+      if (null != leftChild) {
+        evaluateOrExpression(currentNode, leftChild);
+      }
+      if (null != rightChild) {
+        evaluateOrExpression(currentNode, rightChild);
+      }
+    } else {
+      for (Expression exp : currentNode.getChildren()) {
+        if (null != exp) {
+          fillExpressionMap(filterExpressionMap, exp, currentNode);
         }
       }
     }
diff --git a/integration/spark-common-test/src/test/scala/org/apache/carbondata/spark/testsuite/detailquery/RangeFilterTestCase.scala b/integration/spark-common-test/src/test/scala/org/apache/carbondata/spark/testsuite/detailquery/RangeFilterTestCase.scala
index cd7f7fc..e9a83ec 100644
--- a/integration/spark-common-test/src/test/scala/org/apache/carbondata/spark/testsuite/detailquery/RangeFilterTestCase.scala
+++ b/integration/spark-common-test/src/test/scala/org/apache/carbondata/spark/testsuite/detailquery/RangeFilterTestCase.scala
@@ -39,6 +39,20 @@ class RangeFilterTestCase extends QueryTest with BeforeAndAfterAll {
     sql("drop table if exists DICTIONARY_CARBON_6")
     sql("drop table if exists NO_DICTIONARY_CARBON_7")
     sql("drop table if exists NO_DICTIONARY_HIVE_8")
+    sql("drop table if exists carbontest")
+    sql("drop table if exists carbontest_hive")
+    sql(
+      "create table carbontest(c1 string, c2 string, c3 int) stored by 'carbondata' tblproperties" +
+      "('sort_columns'='c3')")
+    (0 to 10).foreach { index =>
+      sql(s"insert into carbontest select '$index','$index',$index")
+    }
+    sql(
+      "create table carbontest_hive(c1 string, c2 string, c3 int) row format delimited fields " +
+      "terminated by ',' tblproperties('sort_columns'='c3')")
+    (0 to 10).foreach { index =>
+      sql(s"insert into carbontest_hive select '$index','$index',$index")
+    }
 
     sql("CREATE TABLE NO_DICTIONARY_HIVE_1 (CUST_ID int,CUST_NAME String,ACTIVE_EMUI_VERSION " +
         "string, DOB timestamp, DOJ timestamp, BIGINT_COLUMN1 bigint,BIGINT_COLUMN2 bigint," +
@@ -587,7 +601,31 @@ class RangeFilterTestCase extends QueryTest with BeforeAndAfterAll {
       sql("select empname from NO_DICTIONARY_HIVE_8 where empname <= '107'"))
   }
 
+  test("Range filter with two between clauses") {
+
+    checkAnswer(sql("select * from carbontest where c3 between 2 and 3 or c3 between 3 and 4"),
+      sql("select * from carbontest_hive where c3 between 2 and 3 or c3 between 3 and 4"))
+
+    checkAnswer(sql("select * from carbontest where c3 >= 2 and c3 <= 3 or c3 >= 3 and c3 <= 4"),
+      sql("select * from carbontest_hive where c3 >= 2 and c3 <= 3 or c3 >= 3 and c3 <= 4"))
+
+    checkAnswer(sql(
+      "select * from carbontest where (c3 between 2 and 3 or c3 between 3 and 4) and (c3 between " +
+      "2 and 4 or c3 between 4 and 5)"),
+      sql(
+        "select * from carbontest_hive where (c3 between 2 and 3 or c3 between 3 and 4) and (c3 " +
+        "between 2 and 4 or c3 between 4 and 5)"))
+
+    checkAnswer(sql(
+      "select * from carbontest where c3 >= 2 and c3 <= 5 and (c3 between 1 and 3 or c3 between 3" +
+      " and 6)"),
+      sql("select * from carbontest_hive where c3 >= 2 and c3 <= 5 and (c3 between 1 and 3 or c3 " +
+        "between 3 and 6)"))
+  }
+
   override def afterAll {
+    sql("drop table if exists carbontest")
+    sql("drop table if exists carbontest_hive")
     sql("drop table if exists filtertestTable")
     sql("drop table if exists NO_DICTIONARY_HIVE_1")
     sql("drop table if exists NO_DICTIONARY_CARBON_1")