[GitHub] carbondata pull request #1650: [CARBONDATA-1703] Refactored code for creatio...

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/1650#discussion_r157416588
 
    --- Diff: integration/spark2/src/main/scala/org/apache/spark/sql/optimizer/CarbonFilters.scala ---
    @@ -78,9 +78,17 @@ object CarbonFilters {
               Some(new LessThanEqualToExpression(getCarbonExpression(name),
                 getCarbonLiteralExpression(name, value)))
             case sources.In(name, values) =>
    -          Some(new InExpression(getCarbonExpression(name),
    -            new ListExpression(
    -              convertToJavaList(values.map(f => getCarbonLiteralExpression(name, f)).toList))))
    +          if (values.length == 1 && values(0) == null) {
    +            Some(new InExpression(getCarbonExpression(name),
    +              new ListExpression(
    --- End diff --
   
    The 1 condition was added when we have only NULL in our IN expression for that case, the Left expression was becoming NULL. So to avoid that we added this case.


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/1650
 
    Build Failed with Spark 2.2.0, Please check CI http://88.99.58.216:8080/job/ApacheCarbonPRBuilder/861/



---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

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



---

[qiuchenjian-2]

Github user geetikagupta16 commented on the issue:

    https://github.com/apache/carbondata/pull/1650
 
    retest this please


---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

    https://github.com/apache/carbondata/pull/1650
 
    SDV Build Fail , Please check CI http://144.76.159.231:8080/job/ApacheSDVTests/2392/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/1650
 
    Build Failed with Spark 2.2.0, Please check CI http://88.99.58.216:8080/job/ApacheCarbonPRBuilder/875/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/1650
 
    Build Success with Spark 2.2.0, Please check CI http://88.99.58.216:8080/job/ApacheCarbonPRBuilder/878/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/1650
 
    Build Success with Spark 2.2.0, Please check CI http://88.99.58.216:8080/job/ApacheCarbonPRBuilder/945/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user geetikagupta16 commented on the issue:

    https://github.com/apache/carbondata/pull/1650
 
    @gvramana Please review again, as I have made the required changes


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/1650#discussion_r158826285
 
    --- Diff: integration/spark-common-test/src/test/scala/org/apache/carbondata/spark/testsuite/detailquery/ExpressionWithNullTestCase.scala ---
    @@ -75,6 +78,9 @@ class ExpressionWithNullTestCase extends QueryTest with BeforeAndAfterAll {
         checkAnswer(sql("select * from expression_test where id not in ('2')"), sql("select * from expression_test_hive where id not in ('2')"))
         checkAnswer(sql("select * from expression_test where id not in (cast('2' as int))"), sql("select * from expression_test_hive where id not in (cast('2' as int))"))
     //    checkAnswer(sql("select * from expression_test where id not in (cast('null' as int))"), sql("select * from expression_test_hive where id not in (cast('null' as int))"))
    --- End diff --
   
    Please uncomment the test cases and rerun it once.


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/1650#discussion_r158827952
 
    --- Diff: integration/spark2/src/main/scala/org/apache/spark/sql/optimizer/CarbonFilters.scala ---
    @@ -78,13 +78,25 @@ object CarbonFilters {
               Some(new LessThanEqualToExpression(getCarbonExpression(name),
                 getCarbonLiteralExpression(name, value)))
             case sources.In(name, values) =>
    -          Some(new InExpression(getCarbonExpression(name),
    -            new ListExpression(
    -              convertToJavaList(values.map(f => getCarbonLiteralExpression(name, f)).toList))))
    +          if (values.length == 1 && values(0) == null) {
    +            Some(new InExpression(getCarbonExpression(name),
    +              new ListExpression(
    +                convertToJavaList(values.map(filterValues =>
    +                  getCarbonLiteralExpression(name, filterValues)).toList))))
    +          } else {
    +            Some(new InExpression(getCarbonExpression(name),
    +              new ListExpression(
    +                convertToJavaList(values.filterNot(_ == null)
    +                  .map(filterValues => getCarbonLiteralExpression(name, filterValues)).toList))))
    +          }
             case sources.Not(sources.In(name, values)) =>
    -          Some(new NotInExpression(getCarbonExpression(name),
    -            new ListExpression(
    -              convertToJavaList(values.map(f => getCarbonLiteralExpression(name, f)).toList))))
    +          if (values.contains(null)) {
    +            Some(new FalseExpression(getCarbonExpression(name)))
    +          } else {
    +            Some(new NotInExpression(getCarbonExpression(name),
    --- End diff --
   
    I think similar changes are required in transformExpression also. In case Cast is not resolved by Spark then transformExpression will handle those expression. But prior doing any changes we need supporting test cases.


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/1650#discussion_r158826908
 
    --- Diff: integration/spark-common-test/src/test/scala/org/apache/carbondata/spark/testsuite/detailquery/ExpressionWithNullTestCase.scala ---
    @@ -75,6 +78,9 @@ class ExpressionWithNullTestCase extends QueryTest with BeforeAndAfterAll {
         checkAnswer(sql("select * from expression_test where id not in ('2')"), sql("select * from expression_test_hive where id not in ('2')"))
         checkAnswer(sql("select * from expression_test where id not in (cast('2' as int))"), sql("select * from expression_test_hive where id not in (cast('2' as int))"))
     //    checkAnswer(sql("select * from expression_test where id not in (cast('null' as int))"), sql("select * from expression_test_hive where id not in (cast('null' as int))"))
    +    checkAnswer(sql("select * from expression_test where id not in (1,2,NULL)"), sql("select * from expression_test_hive where id not in (1,2,NULL)"))
    +    checkAnswer(sql("select * from expression_test where id not in (NULL)"), sql("select * from expression_test_hive where id not in (NULL)"))
    --- End diff --
   
    Please add some more test cases with explicit cast and with Nulls


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/1650#discussion_r158826141
 
    --- Diff: integration/spark2/src/main/scala/org/apache/spark/sql/optimizer/CarbonFilters.scala ---
    @@ -78,13 +78,25 @@ object CarbonFilters {
               Some(new LessThanEqualToExpression(getCarbonExpression(name),
                 getCarbonLiteralExpression(name, value)))
             case sources.In(name, values) =>
    -          Some(new InExpression(getCarbonExpression(name),
    -            new ListExpression(
    -              convertToJavaList(values.map(f => getCarbonLiteralExpression(name, f)).toList))))
    +          if (values.length == 1 && values(0) == null) {
    +            Some(new InExpression(getCarbonExpression(name),
    --- End diff --
   
    In case there are only one value in In list and that is also qualifies as Null, then why cant we return False expression just like Not In case. i.e. Some(new FalseExpression(getCarbonExpression(name))).
   
   



---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/1650#discussion_r158903934
 
    --- Diff: integration/spark2/src/main/scala/org/apache/spark/sql/optimizer/CarbonFilters.scala ---
    @@ -78,13 +78,25 @@ object CarbonFilters {
               Some(new LessThanEqualToExpression(getCarbonExpression(name),
                 getCarbonLiteralExpression(name, value)))
             case sources.In(name, values) =>
    -          Some(new InExpression(getCarbonExpression(name),
    -            new ListExpression(
    -              convertToJavaList(values.map(f => getCarbonLiteralExpression(name, f)).toList))))
    +          if (values.length == 1 && values(0) == null) {
    +            Some(new InExpression(getCarbonExpression(name),
    +              new ListExpression(
    +                convertToJavaList(values.map(filterValues =>
    +                  getCarbonLiteralExpression(name, filterValues)).toList))))
    +          } else {
    +            Some(new InExpression(getCarbonExpression(name),
    +              new ListExpression(
    +                convertToJavaList(values.filterNot(_ == null)
    +                  .map(filterValues => getCarbonLiteralExpression(name, filterValues)).toList))))
    +          }
             case sources.Not(sources.In(name, values)) =>
    -          Some(new NotInExpression(getCarbonExpression(name),
    -            new ListExpression(
    -              convertToJavaList(values.map(f => getCarbonLiteralExpression(name, f)).toList))))
    +          if (values.contains(null)) {
    +            Some(new FalseExpression(getCarbonExpression(name)))
    +          } else {
    +            Some(new NotInExpression(getCarbonExpression(name),
    --- End diff --
   
    Can you please provide scenario for the same


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/1650
 
    Build Failed with Spark 2.2.0, Please check CI http://88.99.58.216:8080/job/ApacheCarbonPRBuilder/1171/



---