Daniel Becker has posted comments on this change. ( http://gerrit.cloudera.org:8080/17847 )
Change subject: IMPALA-10838: Error when struct returned from WITH() ...................................................................... Patch Set 15: (10 comments) http://gerrit.cloudera.org:8080/#/c/17847/14/fe/src/main/java/org/apache/impala/analysis/Expr.java File fe/src/main/java/org/apache/impala/analysis/Expr.java: http://gerrit.cloudera.org:8080/#/c/17847/14/fe/src/main/java/org/apache/impala/analysis/Expr.java@1145 PS14, Line 1145: substituteImplOnChildren(sma > nit. May be renamed as substituteImplOnChildren() Done http://gerrit.cloudera.org:8080/#/c/17847/14/fe/src/main/java/org/apache/impala/analysis/ExprSubstitutionMap.java File fe/src/main/java/org/apache/impala/analysis/ExprSubstitutionMap.java: http://gerrit.cloudera.org:8080/#/c/17847/14/fe/src/main/java/org/apache/impala/analysis/ExprSubstitutionMap.java@239 PS14, Line 239: // Struct children are allowed to be non-materialised because the query may only : // concern a subset of the fields of the struct. > It is not clear to me why we still keep a struct child if it is not a conce It is not the child but the struct that we keep. Without this, we would remove a struct that has at least one non-materialised child. It is important for example in the case of the example query in the commit message, when there is a struct in the inline view and only a part of it is needed in the main query. Only the part needed in the main query is materialised. If we didn't make structs an exception here, the whole struct would be deleted. http://gerrit.cloudera.org:8080/#/c/17847/14/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/14/fe/src/main/java/org/apache/impala/analysis/InlineViewRef.java@268 PS14, Line 268: putStructMembersIntoSmap( > line too long (92 > 90) Done http://gerrit.cloudera.org:8080/#/c/17847/14/fe/src/main/java/org/apache/impala/analysis/InlineViewRef.java@269 PS14, Line 269: slotDesc.getParent().getAlias(), smap_, (SlotRef) colExpr); > line too long (99 > 90) Done http://gerrit.cloudera.org:8080/#/c/17847/14/fe/src/main/java/org/apache/impala/analysis/InlineViewRef.java@288 PS14, Line 288: } > line too long (106 > 90) Done http://gerrit.cloudera.org:8080/#/c/17847/14/fe/src/main/java/org/apache/impala/analysis/Path.java File fe/src/main/java/org/apache/impala/analysis/Path.java: http://gerrit.cloudera.org:8080/#/c/17847/14/fe/src/main/java/org/apache/impala/analysis/Path.java@480 PS14, Line 480: prefixPath.size() > nit. I wonder what happens when this argument has a value larger than the l List.subList would throw an exception but we check it on L479 so it is guaranteed that we don't throw. http://gerrit.cloudera.org:8080/#/c/17847/14/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/14/fe/src/main/java/org/apache/impala/analysis/SlotDescriptor.java@186 PS14, Line 186: if (tupleDescs != null) > nit. This may not be false in all situations since tupleDesc != null is gua In the WHILE loop we check 'tupleDesc' of type TupleDescriptor, here it is 'tupleDescs', which is a List<TupleDescriptor>. http://gerrit.cloudera.org:8080/#/c/17847/14/fe/src/main/java/org/apache/impala/analysis/SlotDescriptor.java@188 PS14, Line 188: parentStructSl > nit. may be renamed as parentSlotDesc. Renamed it to parentStructSlotDesc as I think it is important that it should contain 'struct'. http://gerrit.cloudera.org:8080/#/c/17847/14/fe/src/main/java/org/apache/impala/analysis/SlotDescriptor.java@220 PS14, Line 220: lotDescript > nit. May be renamed as getMaterizaliedSlotSize(). Done http://gerrit.cloudera.org:8080/#/c/17847/14/fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java File fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java: http://gerrit.cloudera.org:8080/#/c/17847/14/fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java@807 PS14, Line 807: getRowSize > nit. May rename as get1stRowSize() as there can be many row-size fields in For more suggestions, see the comment on patch set 10. -- 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: 15 Gerrit-Owner: Daniel Becker <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Daniel Becker <[email protected]> Gerrit-Reviewer: Gabor Kaszab <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Qifan Chen <[email protected]> Gerrit-Comment-Date: Mon, 07 Feb 2022 14:28:42 +0000 Gerrit-HasComments: Yes
