Daniel Becker has posted comments on this change. ( http://gerrit.cloudera.org:8080/17983 )
Change subject: IMPALA-10920: Zipping unnest for arrays ...................................................................... Patch Set 5: (9 comments) Just a few notes. http://gerrit.cloudera.org:8080/#/c/17983/5/be/src/exec/unnest-node.h File be/src/exec/unnest-node.h: http://gerrit.cloudera.org:8080/#/c/17983/5/be/src/exec/unnest-node.h@46 PS5, Line 46: s Nit: typo 'handled'. http://gerrit.cloudera.org:8080/#/c/17983/2/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/17983/2/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@966 PS2, Line 966: Expr; > I found a use case where collExpr is not a SlotRef. I used to have a Precon Ok, thanks. http://gerrit.cloudera.org:8080/#/c/17983/2/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java File fe/src/main/java/org/apache/impala/analysis/SelectStmt.java: http://gerrit.cloudera.org:8080/#/c/17983/2/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java@389 PS2, Line 389: typed columns because " > Yes, a re-analysis is required. In PS3 I did a change that forces re-analys Thanks. http://gerrit.cloudera.org:8080/#/c/17983/5/fe/src/main/java/org/apache/impala/analysis/StmtRewriter.java File fe/src/main/java/org/apache/impala/analysis/StmtRewriter.java: http://gerrit.cloudera.org:8080/#/c/17983/5/fe/src/main/java/org/apache/impala/analysis/StmtRewriter.java@1870 PS5, Line 1870: if (unnestTableRefs.size() == 0) return; Is this line necessary? If the size is 0 the precondition below cannot fail and the and the loop will just exit with zero iterations. http://gerrit.cloudera.org:8080/#/c/17983/2/fe/src/main/java/org/apache/impala/analysis/UnnestExpr.java File fe/src/main/java/org/apache/impala/analysis/UnnestExpr.java: http://gerrit.cloudera.org:8080/#/c/17983/2/fe/src/main/java/org/apache/impala/analysis/UnnestExpr.java@36 PS2, Line 36: > The path has to point to an array and not to it's items. I path contains an Thanks. http://gerrit.cloudera.org:8080/#/c/17983/2/fe/src/main/java/org/apache/impala/analysis/UnnestExpr.java@42 PS2, Line 42: > Agreed. Meanwhile I removed the support for arr.item where it is not an arr If the support for arr.item is removed, we should modify this comment to reflect that. http://gerrit.cloudera.org:8080/#/c/17983/2/fe/src/main/java/org/apache/impala/analysis/UnnestExpr.java@68 PS2, Line 68: // 'rawPath_' points to an array and we need a SlotRef to refer to the item of the > In resolveAndVerifyRawPath() we check that the given path points to an arra Ok, thanks. http://gerrit.cloudera.org:8080/#/c/17983/5/fe/src/main/java/org/apache/impala/analysis/UnnestExpr.java File fe/src/main/java/org/apache/impala/analysis/UnnestExpr.java: http://gerrit.cloudera.org:8080/#/c/17983/5/fe/src/main/java/org/apache/impala/analysis/UnnestExpr.java@31 PS5, Line 31: n Nit: typo 'a'. http://gerrit.cloudera.org:8080/#/c/17983/2/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/17983/2/fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java@1060 PS2, Line 1060: refs.removeAll(reducedCollectionRefs); > You can't provide the same collection multiple times in the unnest so here Thanks. -- To view, visit http://gerrit.cloudera.org:8080/17983 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic58ff6579ecff03962e7a8698edfbe0684ce6cf7 Gerrit-Change-Number: 17983 Gerrit-PatchSet: 5 Gerrit-Owner: Gabor Kaszab <gaborkas...@cloudera.com> Gerrit-Reviewer: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Daniel Becker <daniel.bec...@cloudera.com> Gerrit-Reviewer: Gabor Kaszab <gaborkas...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Comment-Date: Wed, 10 Nov 2021 11:15:06 +0000 Gerrit-HasComments: Yes
