Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/17847 )
Change subject: IMPALA-10838: Error when struct returned from WITH() ...................................................................... Patch Set 23: (7 comments) Looks good to me in general, but I couldn't figure out the explicit questions to the reviewers yet. http://gerrit.cloudera.org:8080/#/c/17847/21//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/17847/21//COMMIT_MSG@33 PS21, Line 33: This change also changes the way struct fields are materialised: until optional: I think that this is more like the main change, so the title could reflect this http://gerrit.cloudera.org:8080/#/c/17847/23//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/17847/23//COMMIT_MSG@7 PS23, Line 7: Error when struct returned from WITH() I think the the title no longer describes what the commit is about. What do you think about "Improve substitution and unification of struct slots"? http://gerrit.cloudera.org:8080/#/c/17847/23/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/17847/23/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java@343 PS23, Line 343: if (whereClause_ != null) { : exprs = Stream.concat(exprs, Stream.of(whereClause_)); : } : : if (havingClause_ != null) { : exprs = Stream.concat(exprs, Stream.of(havingClause_)); : } : : if (groupingExprs_ != null) { : exprs = Stream.concat(exprs, groupingExprs_.stream()); : } : : if (sortInfo_ != null) { : exprs = Stream.concat(exprs, sortInfo_.getSortExprs().stream()); : } : : if (analyticInfo_ != null) { : exprs = Stream.concat(exprs, : analyticInfo_.getAnalyticExprs().stream() : .map(analyticExpr -> (Expr) analyticExpr)); : } This could go to a function like CollectExprsOutsideSelectList http://gerrit.cloudera.org:8080/#/c/17847/23/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java@369 PS23, Line 369: .filter(path -> path != null) : .filter(path -> path.destType().isStructType()) : .collect(Collectors.toList()); nit: indentation http://gerrit.cloudera.org:8080/#/c/17847/23/fe/src/main/java/org/apache/impala/analysis/SlotDescriptor.java File fe/src/main/java/org/apache/impala/analysis/SlotDescriptor.java: http://gerrit.cloudera.org:8080/#/c/17847/23/fe/src/main/java/org/apache/impala/analysis/SlotDescriptor.java@208 PS23, Line 208: public List<SlotDescriptor> getEnclosingStructSlotDescs() { : List<SlotDescriptor> result = new ArrayList<>(); : getEnclosingStructSlotAndTupleDescs(result, null); : return result; : } : : public List<TupleDescriptor> getEnclosingTupleDescs() { : List<TupleDescriptor> result = new ArrayList<>(); : getEnclosingStructSlotAndTupleDescs(null, result); : return result; : } Can you add some commments, e.g. for a struct S's member M it will return the descs of the given structs. http://gerrit.cloudera.org:8080/#/c/17847/23/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/17847/23/fe/src/main/java/org/apache/impala/analysis/SlotRef.java@400 PS23, Line 400: itemTupleDesc Is it possible for the time tuple desc to be NULL? http://gerrit.cloudera.org:8080/#/c/17847/23/testdata/workloads/functional-query/queries/QueryTest/nested-struct-in-select-list.test File testdata/workloads/functional-query/queries/QueryTest/nested-struct-in-select-list.test: http://gerrit.cloudera.org:8080/#/c/17847/23/testdata/workloads/functional-query/queries/QueryTest/nested-struct-in-select-list.test@125 PS23, Line 125: t id, outer_struct from Can you add a few tests where the inline view itself has both a struct and and one of its members in the select list? -- To view, visit http://gerrit.cloudera.org:8080/17847 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iadb9233677355b85d424cc3f22b00b5a3bf61c57 Gerrit-Change-Number: 17847 Gerrit-PatchSet: 23 Gerrit-Owner: Daniel Becker <daniel.bec...@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-Reviewer: Qifan Chen <qc...@cloudera.com> Gerrit-Comment-Date: Mon, 25 Apr 2022 09:13:16 +0000 Gerrit-HasComments: Yes
