wangsheng has posted comments on this change. ( http://gerrit.cloudera.org:8080/18023 )
Change subject: IMPALA-7942 (part 2): Add query hints for predicate selectivities ...................................................................... Patch Set 29: (3 comments) http://gerrit.cloudera.org:8080/#/c/18023/29/fe/src/main/java/org/apache/impala/analysis/Expr.java File fe/src/main/java/org/apache/impala/analysis/Expr.java: http://gerrit.cloudera.org:8080/#/c/18023/29/fe/src/main/java/org/apache/impala/analysis/Expr.java@1064 PS29, Line 1064: if (! predicateHintValid(this)) { > Consider Expr.isTriviallyTrue() for example in the same file. That currentl Hi Kurt, I think you are right. I will discuss with other people to find a better way to solve this problem. http://gerrit.cloudera.org:8080/#/c/18023/29/fe/src/main/java/org/apache/impala/analysis/Expr.java@1064 PS29, Line 1064: if (! predicateHintValid(this)) { > The concern makes sense to me. Expr#getConjuncts() is widely used. Chaning Hi Quanlong, you are right, this change only required by hints on compound predicates. First version of this patch only supported single predicate. Qifan suggested that we'd better support compound predicates as well which are often used in prod env, so I try to add this in later change. I will discuss with Qifan again, maybe we do not provide hint for AND compound predicates in this patch. http://gerrit.cloudera.org:8080/#/c/18023/29/fe/src/main/java/org/apache/impala/analysis/Expr.java@1067 PS29, Line 1067: // Selectivity hint cannot be set. If Predicate been set selectivity hint, we return : // itself directly, such as CompoundPredicate. Otherwise, hint value will missing. > This comment is not accurate and should be removed. The original comment f Hi Qifan. What do you mean 'Note a Predicate contains the selectivityHint_ as the new data member.'? And 'If the above is true, then we do not need getLocalConjuncts() at all.'? I don't understand. If we do not add hint check in 'getConjuncts()', hint for AND compound predicates will invalid, but still valid for OR compound predicates. So maybe we have these options: 1. Remove hint check directly, do not support hint for AND compound predicates, and maybe give some warnings; 2. Transfer hint value to AND compound predicate's children, but how to decide the hint value of each child is a problem. Here is my opinion, how do you think? -- To view, visit http://gerrit.cloudera.org:8080/18023 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2776b9bbd878b8a21d9c866b400140a454f59e1b Gerrit-Change-Number: 18023 Gerrit-PatchSet: 29 Gerrit-Owner: wangsheng <[email protected]> Gerrit-Reviewer: Amogh Margoor <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Kurt Deschler <[email protected]> Gerrit-Reviewer: Qifan Chen <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Xiang Yang <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Reviewer: wangsheng <[email protected]> Gerrit-Comment-Date: Tue, 11 Apr 2023 16:25:31 +0000 Gerrit-HasComments: Yes
