Gabor Kaszab has posted comments on this change. ( http://gerrit.cloudera.org:8080/18094 )
Change subject: IMPALA-11038: Zipping unnest from view ...................................................................... Patch Set 6: (14 comments) PS6 is a rebase with master http://gerrit.cloudera.org:8080/#/c/18094/4//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/18094/4//COMMIT_MSG@10 PS4, Line 10: m > Nit: improves. Done http://gerrit.cloudera.org:8080/#/c/18094/4//COMMIT_MSG@16 PS4, Line 16: both unnesting syntaxes > Nit: we could write the the possibilities in parentheses: select list and f Done http://gerrit.cloudera.org:8080/#/c/18094/4/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/18094/4/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@712 PS4, Line 712: it > Could you make it clear that the returned TableRef is not the one passed in Here actually the uniqueAlias is needed from the parameter TableRef. So I changed the function to receive an alias and re-wrote the comment as well. http://gerrit.cloudera.org:8080/#/c/18094/4/fe/src/main/java/org/apache/impala/analysis/CollectionTableRef.java File fe/src/main/java/org/apache/impala/analysis/CollectionTableRef.java: http://gerrit.cloudera.org:8080/#/c/18094/4/fe/src/main/java/org/apache/impala/analysis/CollectionTableRef.java@221 PS4, Line 221: multipl > Nit: typo. Done http://gerrit.cloudera.org:8080/#/c/18094/4/fe/src/main/java/org/apache/impala/analysis/FromClause.java File fe/src/main/java/org/apache/impala/analysis/FromClause.java: http://gerrit.cloudera.org:8080/#/c/18094/4/fe/src/main/java/org/apache/impala/analysis/FromClause.java@149 PS4, Line 149: if (collRef.getCollectionExpr() != null) return; : // Don't do any checks of th > Does this comment refer to L151 or L152? L152. Done http://gerrit.cloudera.org:8080/#/c/18094/4/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/18094/4/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java@467 PS4, Line 467: List<TupleId> zippingUnnestTupleIds = Lists.newArrayList( > Nit: this condition can be checked before getting the zipping unnest tuples Done http://gerrit.cloudera.org:8080/#/c/18094/4/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java@472 PS4, Line 472: nWhereClause) { > We could convert 'zippingUnnestTupleIds' to a List already on L466 so we do Done http://gerrit.cloudera.org:8080/#/c/18094/4/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java@485 PS4, Line 485: fic for zipping unn > Nit: function identifies. Done http://gerrit.cloudera.org:8080/#/c/18094/4/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java@506 PS4, Line 506: ol > Nit: if. Done http://gerrit.cloudera.org:8080/#/c/18094/4/fe/src/main/java/org/apache/impala/analysis/SlotRef.java File fe/src/main/java/org/apache/impala/analysis/SlotRef.java: http://gerrit.cloudera.org:8080/#/c/18094/4/fe/src/main/java/org/apache/impala/analysis/SlotRef.java@378 PS4, Line 378: _ > Nit: space after underscore. Done http://gerrit.cloudera.org:8080/#/c/18094/4/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/18094/4/fe/src/main/java/org/apache/impala/analysis/UnnestExpr.java@43 PS4, Line 43: adding "item" to the path in the first round of analysis here in the : // unnest. > Is this needed or good? When (or if) we add support for nested arrays, this Well, that is some unintentionally left over comment and not valid anymore. Rephrased it. http://gerrit.cloudera.org:8080/#/c/18094/2/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java File fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java: http://gerrit.cloudera.org:8080/#/c/18094/2/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java@888 PS2, Line 888: if (!checkTypeForOverlapPredicate(slotRefInScan.getType(), targetExpr.getType()) > line too long (91 > 90) Done http://gerrit.cloudera.org:8080/#/c/18094/4/fe/src/main/java/org/apache/impala/planner/PlanNode.java File fe/src/main/java/org/apache/impala/planner/PlanNode.java: http://gerrit.cloudera.org:8080/#/c/18094/4/fe/src/main/java/org/apache/impala/planner/PlanNode.java@514 PS4, Line 514: slotRefs); > Isn't the name 'unnestExprs' a bit misleading? As far as understand they ar Indeed. renamed it. http://gerrit.cloudera.org:8080/#/c/18094/4/fe/src/main/java/org/apache/impala/planner/UnnestNode.java File fe/src/main/java/org/apache/impala/planner/UnnestNode.java: http://gerrit.cloudera.org:8080/#/c/18094/4/fe/src/main/java/org/apache/impala/planner/UnnestNode.java@128 PS4, Line 128: ls.getPathSql(t2.getPath()); > Nit: if path1 is extracted to a variable, so could also "path2". Done -- To view, visit http://gerrit.cloudera.org:8080/18094 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I68f649dda9e41f257e7f6596193d07b24049f92a Gerrit-Change-Number: 18094 Gerrit-PatchSet: 6 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: Fri, 11 Mar 2022 09:00:14 +0000 Gerrit-HasComments: Yes
