Gabor Kaszab has posted comments on this change. ( http://gerrit.cloudera.org:8080/17847 )
Change subject: IMPALA-10838: Error when struct returned from WITH() ...................................................................... Patch Set 12: (14 comments) http://gerrit.cloudera.org:8080/#/c/17847/12/fe/src/main/java/org/apache/impala/analysis/DescriptorTable.java File fe/src/main/java/org/apache/impala/analysis/DescriptorTable.java: http://gerrit.cloudera.org:8080/#/c/17847/12/fe/src/main/java/org/apache/impala/analysis/DescriptorTable.java@155 PS12, Line 155: they nit: instead of 'they' could you use 'desc'? http://gerrit.cloudera.org:8080/#/c/17847/10/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/10/fe/src/main/java/org/apache/impala/analysis/ExprSubstitutionMap.java@127 PS10, Line 127: final Path structFieldPath = structField.getResolvedPath(); > No, in the sorting tuple it will have no path at all. Indeed, thanks for pointing this out! http://gerrit.cloudera.org:8080/#/c/17847/10/fe/src/main/java/org/apache/impala/analysis/ExprSubstitutionMap.java@189 PS10, Line 189: break; > I don't think so. We take the elements of the raw path one by one and choos but it could be easily checked if we are at the end of the path by comparing 'i' to 'path.size()', right? http://gerrit.cloudera.org:8080/#/c/17847/12/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/12/fe/src/main/java/org/apache/impala/analysis/ExprSubstitutionMap.java@103 PS12, Line 103: a struct field nit: a field of a struct. http://gerrit.cloudera.org:8080/#/c/17847/12/fe/src/main/java/org/apache/impala/analysis/ExprSubstitutionMap.java@105 PS12, Line 105: public Expr getStructFieldThroughEnclosingStruct(SlotRef structField) { Thanks for rewriting the content of this function! I find it way more readable. http://gerrit.cloudera.org:8080/#/c/17847/12/fe/src/main/java/org/apache/impala/analysis/ExprSubstitutionMap.java@110 PS12, Line 110: nit: extra space http://gerrit.cloudera.org:8080/#/c/17847/12/fe/src/main/java/org/apache/impala/analysis/ExprSubstitutionMap.java@122 PS12, Line 122: SlotRef findEnclosingStruct(SlotRef structField) { It's private, right? http://gerrit.cloudera.org:8080/#/c/17847/12/fe/src/main/java/org/apache/impala/analysis/ExprSubstitutionMap.java@131 PS12, Line 131: structFiled nit: typo http://gerrit.cloudera.org:8080/#/c/17847/12/fe/src/main/java/org/apache/impala/analysis/ExprSubstitutionMap.java@142 PS12, Line 142: getRawPathFromParentToChild A different name would be even more verbose. You could emphasise that this is only for structs and not for any kind of parent-child relationship. E.g. the variable name in this line would be a nice option for the function name: getRawPathFromStructToField() http://gerrit.cloudera.org:8080/#/c/17847/12/fe/src/main/java/org/apache/impala/analysis/ExprSubstitutionMap.java@145 PS12, Line 145: Preconditions.checkNotNull(possibleContainingStruct); If 'possibleContainingStruct' was null then we would get an NPE at L155, right? Anyway, in the substitution map it's not possible to have NULLs as LHS values, is it? http://gerrit.cloudera.org:8080/#/c/17847/12/fe/src/main/java/org/apache/impala/analysis/ExprSubstitutionMap.java@179 PS12, Line 179: List<SlotRef> slotRefChildren = nextOnPath.getChildren().stream() I think there is an existing function in Expr called collect() or collectAll() or something where you can provide a type like SlotRef and it gathers all the descendants with the given type. Would that be helpful for you in this case? http://gerrit.cloudera.org:8080/#/c/17847/10/fe/src/main/java/org/apache/impala/analysis/TupleDescriptor.java File fe/src/main/java/org/apache/impala/analysis/TupleDescriptor.java: http://gerrit.cloudera.org:8080/#/c/17847/10/fe/src/main/java/org/apache/impala/analysis/TupleDescriptor.java@137 PS10, Line 137: getSlotsRecursivelyHelper(result); > Then we would need to allocate temporary lists at each level and merge them I guess the addAll() function copies the references only and not the whole objects so for me it doesn't seem like a big perf impact. Would make the code a bit more compact at the callsite. http://gerrit.cloudera.org:8080/#/c/17847/10/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/10/fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java@817 PS10, Line 817: Assert.fail("Failed to create exec request for '" + query + "': " + e.getMessage()); > Yes, it is from the scan node and you're right it wouldn't work with more c I was wondering if this row size check should be done as you do it from Java code or should it be a comparison of the explain output in the .test files. In the latter case you would have to provide the full plan output that seems an overkill a bit but should be more straightforward what we compare with what. What do you think? http://gerrit.cloudera.org:8080/#/c/17847/9/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/9/testdata/workloads/functional-query/queries/QueryTest/nested-struct-in-select-list.test@260 PS9, Line 260: ==== > Do you think we should change the error message? Should it be done in a sep I'd vote for not doing this message change in this patch as it is not relevant in this context in my opinion. -- 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: 12 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-Comment-Date: Mon, 31 Jan 2022 12:46:09 +0000 Gerrit-HasComments: Yes
