[GitHub] carbondata pull request #2665: [CARBONDATA-2897][DataMap] Optimize datamap c...

[qiuchenjian-2]

GitHub user kevinjmh opened a pull request:

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

    [CARBONDATA-2897][DataMap] Optimize datamap chooser

    In this PR,
    1. Remove code for merging into one datamap when some datamap hits both child nodes of And/Or expression in DataMapChooser. This aims to make datamap focus on pruning single index column without any logic process. Leave logic stuff to be done by AndDataMapExprWrapper and OrDataMapExprWrapper
    2. Only extract ColumnExpression of Expression which our datamap can handle in DataMapChooser.
    3. Add short circuit to return pruned result when result of left node is empty in AndDataMapExprWrapper.
   
   
   
    Be sure to do all of the following checklist to help us incorporate
    your contribution quickly and easily:
   
     - [ ] Any interfaces changed?
     
     - [ ] Any backward compatibility impacted?
     
     - [ ] Document update required?
   
     - [ ] Testing done
            Please provide details on
            - Whether new unit test cases have been added or why no new tests are required?
            - How it is tested? Please attach test report.
            - Is it a performance related change? Please attach the performance test report.
            - Any additional information to help reviewers in testing this change.
           
     - [ ] 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/kevinjmh/carbondata dmChooser

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

    https://github.com/apache/carbondata/pull/2665.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 #2665
   
----
commit 2c03e75f7d77df7609f2b054dcf93bdd7256bfa8
Author: Manhua <kevinjmh@...>
Date:   2018-08-28T11:51:52Z

    datamap chooser

----


---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2665
 
    Build Failed with Spark 2.2.1, Please check CI http://95.216.28.178:8080/job/ApacheCarbonPRBuilder1/51/



---

[qiuchenjian-2]

Github user kevinjmh commented on the issue:

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


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2665
 
    Build Success with Spark 2.2.1, Please check CI http://95.216.28.178:8080/job/ApacheCarbonPRBuilder1/60/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2665
 
    Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder2.1/15/



---

[qiuchenjian-2]

Github user QiangCai commented on the issue:

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


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2665
 
    Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder2.1/222/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2665
 
    Build Success with Spark 2.2.1, Please check CI http://95.216.28.178:8080/job/ApacheCarbonPRBuilder1/390/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2665
 
    Build Failed  with Spark 2.3.1, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder2.3/8460/



---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2665#discussion_r217945289
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/datamap/DataMapChooser.java ---
    @@ -268,23 +238,38 @@ private ExpressionTuple selectDataMap(Expression expression, List<TableDataMap>
     
       private void extractColumnExpression(Expression expression,
           List<ColumnExpression> columnExpressions) {
    -    if (expression instanceof ColumnExpression) {
    -      columnExpressions.add((ColumnExpression) expression);
    -    } else if (expression instanceof MatchExpression) {
    -      // this is a special case for lucene
    -      // build a fake ColumnExpression to filter datamaps which contain target column
    -      // a Lucene query string is alike "column:query term"
    -      String[] queryItems = expression.getString().split(":", 2);
    -      if (queryItems.length == 2) {
    -        columnExpressions.add(new ColumnExpression(queryItems[0], null));
    -      }
    -    } else if (expression != null) {
    -      List<Expression> children = expression.getChildren();
    -      if (children != null && children.size() > 0) {
    -        for (Expression exp : children) {
    -          extractColumnExpression(exp, columnExpressions);
    +    switch (expression.getFilterExpressionType()) {
    --- End diff --
   
    I think these operation should not be in DataMapChooser.
    DataMapChooser is for common logic and should not handle specific datamap's logic.
    If you want to decide which expression will be supported by the specific datamap, here I do propose you to refactor the 'SUPPORTED_EXPRESSION' in the specific datamap. In that place, the datamap should declare what kind of operand and operator it will support and in DataMapChooser we just need to call that method and decide which expression will be handled by that datamap.


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2665
 
    Build Failed  with Spark 2.3.1, Please check CI http://136.243.101.176:8080/job/carbondataprbuilder2.3/8589/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2665
 
    Build Failed  with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder2.1/343/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2665
 
    Build Failed  with Spark 2.3.1, Please check CI http://136.243.101.176:8080/job/carbondataprbuilder2.3/8590/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2665
 
    Build Failed with Spark 2.2.1, Please check CI http://95.216.28.178:8080/job/ApacheCarbonPRBuilder1/520/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2665
 
    Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder2.1/344/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2665
 
    Build Failed  with Spark 2.3.1, Please check CI http://136.243.101.176:8080/job/carbondataprbuilder2.3/8591/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2665
 
    Build Failed with Spark 2.2.1, Please check CI http://95.216.28.178:8080/job/ApacheCarbonPRBuilder1/521/



---