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 17: (6 comments) http://gerrit.cloudera.org:8080/#/c/17847/17/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/17847/17/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@1493 PS17, Line 1493: // TODO: Unify with registerNewSlotRef(). Do you want to resolve this before merging the change? The only difference I spotted is using getFullyQualifiedRawPath() instead if slotPath - can you add a comment about why is this needed? http://gerrit.cloudera.org:8080/#/c/17847/17/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@1505 PS17, Line 1505: stream This looks a bit expensive to me - we do it with all slotRefs (with the exception of arrays), and has size O(num_slotrefs). Couldn`t we maintain a list of path that either have a struct parent or are structs, and do the operations below based on that? http://gerrit.cloudera.org:8080/#/c/17847/17/fe/src/main/java/org/apache/impala/analysis/InlineViewRef.java File fe/src/main/java/org/apache/impala/analysis/InlineViewRef.java: http://gerrit.cloudera.org:8080/#/c/17847/17/fe/src/main/java/org/apache/impala/analysis/InlineViewRef.java@277 PS17, Line 277: the indentation change seems off http://gerrit.cloudera.org:8080/#/c/17847/17/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/17/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java@344 PS17, Line 344: // TODO: Review question: shouldn't we include the expressions from the WHERE, ORDER : // BY etc. clauses anyway so that when we add support for struct UDFs, those work : // out of the box? Yes, we should handle those in the long run. Maybe it would be easier to handle this by registering or struct/child of struct slot ref during analyses, and try to merge them in a later step? http://gerrit.cloudera.org:8080/#/c/17847/17/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java@355 PS17, Line 355: .filter(path -> path != null) nit: indentation http://gerrit.cloudera.org:8080/#/c/17847/16/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/16/testdata/workloads/functional-query/queries/QueryTest/nested-struct-in-select-list.test@212 PS16, Line 212: # WITH clause creates an inline view containing a two structs; select only one of them and Can you add some tests where a struct member is used in the ON clause + it is returned also returned in the select list? I became a bit worried about this: https://github.com/apache/impala/blob/master/fe%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fimpala%2Fanalysis%2FTableRef.java#L689 We collect the tuples ids from an expression during analyses - won't this return the tuple id of the struct tuple instead of the base table tuple? -- 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: 17 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, 07 Mar 2022 20:20:22 +0000 Gerrit-HasComments: Yes
