Qifan Chen has posted comments on this change. ( http://gerrit.cloudera.org:8080/16266 )
Change subject: IMPALA-5022: Outer join simplification ...................................................................... Patch Set 5: (14 comments) Thanks for the work. 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@3233 PS5, Line 3233: where clause Suggest to remove to make the comment more precise. 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, such as: This method identifies null-rejecting predicates which are the requirements to convert an outer-join to an inner join. http://gerrit.cloudera.org:8080/#/c/16266/5/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@3240 PS5, Line 3240: contains nit. containing http://gerrit.cloudera.org:8080/#/c/16266/5/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@3242 PS5, Line 3242: t1.v1 you mean t2.v2? http://gerrit.cloudera.org:8080/#/c/16266/5/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@3254 PS5, Line 3254: intersect.isEmpty() For any disConjunct, when "ids intersect disConjunct != disConjunct", then disConjuncts should be skipped. The test here seems not sufficient. http://gerrit.cloudera.org:8080/#/c/16266/5/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@3264 PS5, Line 3264: ULL input, eg, ISNULL(), IFNULL(), ZEROIFNULL(). We may need to reject UDFs as these functions can maintain a state which could allow the function to return different outputs for a given input. That is, we can not guarantee that such a UDF would not produce a NULL given a NULL input. http://gerrit.cloudera.org:8080/#/c/16266/5/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@3270 PS5, Line 3270: analyzeNoThrow For some common SQL functions, we probably can directly test their existence and bypass the evaluation logic, assuming the evaluation during compile time is relatively expensive. http://gerrit.cloudera.org:8080/#/c/16266/5/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@3278 PS5, Line 3278: if (!isTr It is a good idea to add a comment here. http://gerrit.cloudera.org:8080/#/c/16266/5/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@3289 PS5, Line 3289: ex); We probably should return false here. http://gerrit.cloudera.org:8080/#/c/16266/5/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@3361 PS5, Line 3361: inner null-filling table http://gerrit.cloudera.org:8080/#/c/16266/5/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@3362 PS5, Line 3362: inner null-filling http://gerrit.cloudera.org:8080/#/c/16266/5/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@3364 PS5, Line 3364: inner null-filling http://gerrit.cloudera.org:8080/#/c/16266/5/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@3365 PS5, Line 3365: null filtering null-rejecting http://gerrit.cloudera.org:8080/#/c/16266/5/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@3375 PS5, Line 3375: case INNER_JOIN: { : break; Probably can be moved to the last 'default' section (of switch). -- 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: 5 Gerrit-Owner: Xianqing He <hexianqing...@126.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com> Gerrit-Reviewer: Xianqing He <hexianqing...@126.com> Gerrit-Comment-Date: Tue, 04 Aug 2020 15:10:27 +0000 Gerrit-HasComments: Yes
