Xianqing He has posted comments on this change. ( http://gerrit.cloudera.org:8080/16266 )
Change subject: IMPALA-5022 part 1: Implement core functions of outer join simplification ...................................................................... Patch Set 22: (10 comments) http://gerrit.cloudera.org:8080/#/c/16266/19/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/19/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@3252 PS19, Line 3252: result; > nit: In the comment above, could you elaborate on the returned set of sets Done http://gerrit.cloudera.org:8080/#/c/16266/19/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@3269 PS19, Line 3269: } > This is O(n^2), although understandably the sizes are small. It seems the c Done http://gerrit.cloudera.org:8080/#/c/16266/19/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@3349 PS19, Line 3349: } > nit: The 'continue' is not needed. Done http://gerrit.cloudera.org:8080/#/c/16266/19/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@3458 PS19, Line 3458: } > Similar to other such try-catch, can you log a warning here ? Also 'conti Done http://gerrit.cloudera.org:8080/#/c/16266/19/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@3475 PS19, Line 3475: } > Can we just propagate the set directly instead of the list ? This will all Done http://gerrit.cloudera.org:8080/#/c/16266/20/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/20/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@3268 PS20, Line 3268: set.add(new HashSet<TupleId>(tids)); > nitpick: would you mind just making intersect() a static member of the Id c Done http://gerrit.cloudera.org:8080/#/c/16266/20/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@3347 PS20, Line 3347: } > Maybe mention "Null Rejecting" somewhere in the error message so it's more Done http://gerrit.cloudera.org:8080/#/c/16266/20/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@3501 PS20, Line 3501: isSimplified = isSimplified ? true : ret; > nitpick the switch indentation doesn't look correct. Trying running clang-f Done http://gerrit.cloudera.org:8080/#/c/16266/10/fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java File fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java: http://gerrit.cloudera.org:8080/#/c/16266/10/fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java@322 PS10, Line 322: functionNameEqualsBuiltin(fnName_, "decode") || > Would please use functionNameEqualsBuiltIn() instead. Feel free to refactor Done http://gerrit.cloudera.org:8080/#/c/16266/20/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/20/fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java@749 PS20, Line 749: && > We should also include a check for straight join hints as it makes sense no 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: 22 Gerrit-Owner: Xianqing He <[email protected]> Gerrit-Reviewer: Aman Sinha <[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: Mon, 21 Sep 2020 11:05:05 +0000 Gerrit-HasComments: Yes
