[GitHub] carbondata pull request #2334: [CARBONDATA-2515][CARBONDATA-2516] fixed Time...

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user sv71294 commented on the issue:

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


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user chenliang613 commented on the issue:

    https://github.com/apache/carbondata/pull/2334
 
    @sv71294  thank you for your contribution.
    In this pr, i didn't find the test case to cover "Timestamp greaterthan expression"


---

[qiuchenjian-2]

Github user chenliang613 commented on the issue:

    https://github.com/apache/carbondata/pull/2334
 
    For "[CARBONDATA-2515] OR filter Expression issue" , verified.
    ```
    presto:default> select * from carbon_table where ID='No.11' and country='country0' and population > 100 or population < 64;
     id | country | city | population
    ----+---------+------+------------
    (0 rows)
    ```
    The PR's result as below :
    ```
    presto:default> select * from carbon_table where population > 100 or population < 64;
        id     | country  |  city  | population
    -----------+----------+--------+------------
     No.0      | country0 | city0  |        200
     No.0      | country0 | city0  |          0
     No.0      | country0 | city0  |          0
     No.0      | country0 | city10 |         60
     No.0      | country0 | city12 |        112
     No.0      | country0 | city12 |        112
     No.0      | country0 | city14 |        264
     No.0      | country0 | city14 |        264
     No.0      | country0 | city14 |        164
     No.0      | country0 | city16 |         16
    ```
   



---

[qiuchenjian-2]

Github user sv71294 commented on the issue:

    https://github.com/apache/carbondata/pull/2334
 
    @chenliang613 adding the test case to cover timestamp greater than expression issue


---

[qiuchenjian-2]

Github user sv71294 commented on the issue:

    https://github.com/apache/carbondata/pull/2334
 
    @chenliang613 I have added the required test case for greater than expresion, please check it now


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user sv71294 commented on the issue:

    https://github.com/apache/carbondata/pull/2334
 
    Retest this please


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

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



---

[qiuchenjian-2]

Github user chenliang613 commented on the issue:

    https://github.com/apache/carbondata/pull/2334
 
    @sv71294  
    i can also run successfully at current master with the below test case, so not sure this test case can cover this issue which be fixed in the PR.
    ```
    test("test the Timestamp greaterthan expression"){
        val actualResult: List[Map[String, Any]] = PrestoServer.executeQuery("SELECT DOB FROM TESTDB.TESTTABLE" +
                                                                             " WHERE DOB > timestamp '2016-01-01 00:00:00.0' order by DOB")
        val expectedResult: List[Map[String, Any]] = List(
          Map("DOB" -> new Timestamp(new java.util.Date(2016-1900,1-1,14,15,7,9).getTime)),
          Map("DOB" -> new Timestamp(new java.util.Date(2016-1900,4-1,14,15,0,9).getTime)))
        assert(actualResult.equals(expectedResult))
      }
    ```


---

[qiuchenjian-2]

Github user sv71294 commented on the issue:

    https://github.com/apache/carbondata/pull/2334
 
    @chenliang613 issue is not about the result, in current master filter is getting handled by presto as it is not generated on connector layer, now with this PR it will be generated at connector layer and passed to core for filter blocklet scanning which will reduce the no of rows passed to presto with filter expression else presto will need to filter the row from universal set.
   
    You can also refer to JIRA ISSUE CARBONDATA-2516 for clarity.


---

[qiuchenjian-2]

Github user sv71294 commented on the issue:

    https://github.com/apache/carbondata/pull/2334
 
    @chenliang613 please check this image, condition logic was missing
    



---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2334#discussion_r197669862
 
    --- Diff: integration/presto/src/main/java/org/apache/carbondata/presto/PrestoFilterUtil.java ---
    @@ -171,134 +170,89 @@ else if (colType.equals(DecimalType.createDecimalType(carbondataColumnHandle.get
        * @return
        */
       static Expression parseFilterExpression(TupleDomain<ColumnHandle> originalConstraint) {
    -    ImmutableList.Builder<Expression> filters = ImmutableList.builder();
     
         Domain domain;
    +    Expression finalFilters = null;
    --- End diff --
   
    Can you provide comments to explain the below Expression?  and why colExpression need put "final"?
   
    Expression finalFilters = null;
    final Expression colExpression = new ColumnExpression(cdch.getColumnName(), coltype);
    Expression colValueExpression = null;
    Expression rangeExpression = null;


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2334#discussion_r197678803
 
    --- Diff: integration/presto/src/main/java/org/apache/carbondata/presto/PrestoFilterUtil.java ---
    @@ -171,134 +170,89 @@ else if (colType.equals(DecimalType.createDecimalType(carbondataColumnHandle.get
        * @return
        */
       static Expression parseFilterExpression(TupleDomain<ColumnHandle> originalConstraint) {
    -    ImmutableList.Builder<Expression> filters = ImmutableList.builder();
     
         Domain domain;
    +    Expression finalFilters = null;
    --- End diff --
   
    1. finalFilters => this one is the final expression for the table,  this variable is returned by the method after combining all the column filters (colValueExpression).
    2. colExpression => we have used final to make sure that value of this variable doesn't get change during the execution of iteration for respective column.
    3. colValueExpression => this is the combination of multiple rangeExpression for a single column in case of multiple filters on single column else this is equal to rangeExpression
    4.rangeExpression => this is generated for each range of column i.e. lessThan, greaterThan, there can be multiple ranges for a single column.


---