Xianqing He has posted comments on this change. ( http://gerrit.cloudera.org:8080/16266 )
Change subject: IMPALA-5022: Outer join simplification ...................................................................... Patch Set 7: (23 comments) http://gerrit.cloudera.org:8080/#/c/16266/5//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/16266/5//COMMIT_MSG@17 PS5, Line 17: one null rejecting condition on the inner table. > Consider adding a query option like DISABLE_OUTER_TO_INNER_REWRITE so disab Done http://gerrit.cloudera.org:8080/#/c/16266/5//COMMIT_MSG@34 PS5, Line 34: * Update the baseline plan Tests > Please try out TPC-DS Q49 there the LOJ queries in there should be rewritte Done http://gerrit.cloudera.org:8080/#/c/16266/5/fe/src/main/java/org/apache/impala/analysis/Analyzer.java File fe/src/main/java/org/apache/impala/analysis/Analyzer.java: http://gerrit.cloudera.org:8080/#/c/16266/5/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@3217 PS5, Line 3217: Option(); > Technically this would include having clause conjuncts as well, so might be Done http://gerrit.cloudera.org:8080/#/c/16266/5/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@3227 PS5, Line 3227: public String getServerName() { > As a further optimization you could use getEquivClassesOnTuples() to also c This optimization is already supported by inner join on clause http://gerrit.cloudera.org:8080/#/c/16266/5/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@3233 PS5, Line 3233: : globalStat > Suggest to remove to make the comment more precise. Done http://gerrit.cloudera.org:8080/#/c/16266/5/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@3234 PS5, Line 3234: > Suggest to add additional comment here to describe the use of the method, s Done http://gerrit.cloudera.org:8080/#/c/16266/5/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@3240 PS5, Line 3240: > nit. containing Done http://gerrit.cloudera.org:8080/#/c/16266/5/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@3242 PS5, Line 3242: > you mean t2.v2? when t2.v2 IS NOT NULL, t1.v1 can be null http://gerrit.cloudera.org:8080/#/c/16266/5/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@3245 PS5, Line 3245: > If e is a conjunct, I think we also need to subject it to the intersection Done http://gerrit.cloudera.org:8080/#/c/16266/5/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@3254 PS5, Line 3254: > For any disConjunct, when "ids intersect disConjunct != disConjunct", then Done http://gerrit.cloudera.org:8080/#/c/16266/5/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@3270 PS5, Line 3270: NULL, because > Agreed it would be good to have some static expressions that we know won't Done http://gerrit.cloudera.org:8080/#/c/16266/5/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@3270 PS5, Line 3270: NULL, because > For some common SQL functions, we probably can directly test their existen Done http://gerrit.cloudera.org:8080/#/c/16266/5/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@3278 PS5, Line 3278: st<TupleI > It is a good idea to add a comment here. Done http://gerrit.cloudera.org:8080/#/c/16266/5/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@3289 PS5, Line 3289: pty( > We probably should return false here. We need skip this, maybe have other null-rejecting conjuncts http://gerrit.cloudera.org:8080/#/c/16266/5/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@3361 PS5, Line 3361: > null-filling table Done http://gerrit.cloudera.org:8080/#/c/16266/5/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@3362 PS5, Line 3362: > null-filling Done http://gerrit.cloudera.org:8080/#/c/16266/5/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@3364 PS5, Line 3364: inedT > null-filling Done http://gerrit.cloudera.org:8080/#/c/16266/5/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@3365 PS5, Line 3365: balState_.full > null-rejecting Done http://gerrit.cloudera.org:8080/#/c/16266/5/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@3375 PS5, Line 3375: } : globalSt > Probably can be moved to the last 'default' section (of switch). Done http://gerrit.cloudera.org:8080/#/c/16266/5/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@3427 PS5, Line 3427: try { > See later comment in Planner, but it might be better to return this and hav Done http://gerrit.cloudera.org:8080/#/c/16266/5/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/16266/5/fe/src/main/java/org/apache/impala/analysis/Expr.java@978 PS5, Line 978: return id_.asInt(); > There is something off about this interface. You assume the caller the firs Done http://gerrit.cloudera.org:8080/#/c/16266/5/fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java File fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java: http://gerrit.cloudera.org:8080/#/c/16266/5/fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java@748 PS5, Line 748: // Transform outer join into inner join whenever possible > Might want to use some state in the analyzer to check if any Outer Joins ex Done http://gerrit.cloudera.org:8080/#/c/16266/5/fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java@749 PS5, Line 749: if (analyzer.getQueryOptions().isEnable_outer_to_inner_rewrites() && > It would be if you returned some indicator that the value transfer graph ne Done -- To view, visit http://gerrit.cloudera.org:8080/16266 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iaa7804033fac68e93f33c387dc68ef67f803e93e Gerrit-Change-Number: 16266 Gerrit-PatchSet: 7 Gerrit-Owner: Xianqing He <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Qifan Chen <[email protected]> Gerrit-Reviewer: Shant Hovsepian <[email protected]> Gerrit-Reviewer: Xianqing He <[email protected]> Gerrit-Comment-Date: Thu, 13 Aug 2020 13:45:17 +0000 Gerrit-HasComments: Yes
