Daniel Becker has posted comments on this change. ( http://gerrit.cloudera.org:8080/18094 )
Change subject: IMPALA-11038: Zipping unnest from view ...................................................................... Patch Set 4: (13 comments) Only some nits, though I haven't gone through the tests yet. Thanks. 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: n Nit: improves. 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 from clause. It may make it easier to understand it for someone that doesn't know the context well. 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 but the one corresponding to it that has been registered? Or are they the same? 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. 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: // Don't do any checks of the collection that came from a view as getTable() would : // return null in that case. Does this comment refer to L151 or L152? 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: if (analyzer_.getNumZippingUnnests() <= 1) return; Nit: this condition can be checked before getting the zipping unnest tuples (on the previous line). http://gerrit.cloudera.org:8080/#/c/18094/4/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java@472 PS4, Line 472: Lists.newArrayList We could convert 'zippingUnnestTupleIds' to a List already on L466 so we don't have to do it in a loop. http://gerrit.cloudera.org:8080/#/c/18094/4/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java@485 PS4, Line 485: functions identifie Nit: function identifies. http://gerrit.cloudera.org:8080/#/c/18094/4/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java@506 PS4, Line 506: is Nit: if. 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. 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: This way "unnest(array_name)" and "unnest(array_name.item)" would : // both work. Is this needed or good? When (or if) we add support for nested arrays, this could lead to confusion as to whether the outer or the inner array is unnested. 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: unnestExprs Isn't the name 'unnestExprs' a bit misleading? As far as understand they are only really unnest expressions if 'zippingUnnestTupleIds.contains()' is true for them. 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: ToSqlUtils.getPathSql(t2.getPath() Nit: if path1 is extracted to a variable, so could also "path2". -- 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: 4 Gerrit-Owner: Gabor Kaszab <gaborkas...@cloudera.com> Gerrit-Reviewer: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Daniel Becker <daniel.bec...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Comment-Date: Mon, 07 Mar 2022 17:17:12 +0000 Gerrit-HasComments: Yes
