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 10: (18 comments) http://gerrit.cloudera.org:8080/#/c/17847/9//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/17847/9//COMMIT_MSG@29 PS9, Line 29: in expression substitutions (in : org.apache.impala.analysis.Expr.substituteImpl), if there is no : substitution mapping for a field of a struct but there is a mapping : for an enclosing struct, find the mapping for that enclosing struct : and from its subexpressions use the one corresponding to the : original expression for substitution > I think both solutions have upsides and downsides. If we have big structs a Thanks for the explanation! Taking a second look, it seems better to load these fields into the subsMap on demand. http://gerrit.cloudera.org:8080/#/c/17847/10/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/10/fe/src/main/java/org/apache/impala/analysis/DescriptorTable.java@156 PS10, Line 156: getEnclosingSlotDescs nit: getEnclosingStructSlotDescs() ? http://gerrit.cloudera.org:8080/#/c/17847/10/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/10/fe/src/main/java/org/apache/impala/analysis/Expr.java@1149 PS10, Line 1149: // SlotRef overrides this method and doesn't call resetAnalysisState(). I'm still wondering if this is the right place for this comment. The Expr class shouldn't know anything about the classes that extend it. E.g. shouldn't know if SlotRef as a child will override resetAnalysisState() or not. This comment should be in SlotRef in my opinion. Or in case you find it important to emphasise the fact here, then the original "if slotref then" logic should be here. 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@104 PS10, Line 104: incldes nit: typo http://gerrit.cloudera.org:8080/#/c/17847/10/fe/src/main/java/org/apache/impala/analysis/ExprSubstitutionMap.java@109 PS10, Line 109: findEnclosingStructAndPathFromIt For me this still seems unnatural to ask 2 things from one function. This finds an enclosing struct for a SlotRef and gets a path from the struct to the field. In my opinion the code would be much readable if this was 2 different steps. I understand that you can easily return but in case this doesn't affect performance I'd prefer to split into 2. http://gerrit.cloudera.org:8080/#/c/17847/10/fe/src/main/java/org/apache/impala/analysis/ExprSubstitutionMap.java@109 PS10, Line 109: FromIt nit: I don't see the purpose of the "FromIt" postfix in the name. http://gerrit.cloudera.org:8080/#/c/17847/10/fe/src/main/java/org/apache/impala/analysis/ExprSubstitutionMap.java@120 PS10, Line 120: Pair<SlotRef, List<String> Could you please write a comment for the function to make it clear that the List<String> is a raw path? http://gerrit.cloudera.org:8080/#/c/17847/10/fe/src/main/java/org/apache/impala/analysis/ExprSubstitutionMap.java@120 PS10, Line 120: private Pair<SlotRef, List<String>> nit: I wouldn't break the line before the function name but would move the params to a new line. Additionally, ident with 4 spaces after the linebreak. http://gerrit.cloudera.org:8080/#/c/17847/10/fe/src/main/java/org/apache/impala/analysis/ExprSubstitutionMap.java@127 PS10, Line 127: if (structFieldPath == null) return null; If 'structField' is analysed then it will always have a non-null resolvedPath, right? http://gerrit.cloudera.org:8080/#/c/17847/10/fe/src/main/java/org/apache/impala/analysis/ExprSubstitutionMap.java@136 PS10, Line 136: final Expr possibleContainingStruct = getLhs().get(i); Shouldn't we check if possibleContainingStruct is indeed a struct? http://gerrit.cloudera.org:8080/#/c/17847/10/fe/src/main/java/org/apache/impala/analysis/ExprSubstitutionMap.java@154 PS10, Line 154: getPathFromParentToChild nit: name suggestion: getRawPathFromParentToChild http://gerrit.cloudera.org:8080/#/c/17847/10/fe/src/main/java/org/apache/impala/analysis/ExprSubstitutionMap.java@163 PS10, Line 163: // Returns null if 'parentPath' is not actually a prefix of 'childPath', i.e. I think it's a bad sign of a wrongly picked function name that you have to explain the behaviour on the callsite. A suggestion for the name: getRawPathWithoutPrefix(Path) Here, it's also obvious what the return type would be, and that 'childPath' is not going to change. What do you think? http://gerrit.cloudera.org:8080/#/c/17847/10/fe/src/main/java/org/apache/impala/analysis/ExprSubstitutionMap.java@175 PS10, Line 175: SlotRef nextOnPath = enclosingStruct; Pls add a precondition that 'enclosingStruct' is struct. http://gerrit.cloudera.org:8080/#/c/17847/10/fe/src/main/java/org/apache/impala/analysis/ExprSubstitutionMap.java@189 PS10, Line 189: foundNextChild = true; Just think out loud: Could you get rid of the 'foundNextChild' var by returning here if we reached the end of the raw path in 'path'? http://gerrit.cloudera.org:8080/#/c/17847/10/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/10/fe/src/main/java/org/apache/impala/analysis/Path.java@477 PS10, Line 477: removePrefix nit: just wondering: would it be more clear what this function does if we named it something like getPathWithoutPrefix(). Then it's obvious that it doesn't change 'this'. Update: see my comment at the callsite. 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); I think you can implement this in a way where you get the result by return type and not by parameter. In that case you could also merge this line with the one below. 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: Pattern rowSizePattern = Pattern.compile("row-size=([0-9]*)B"); Here we try to read the row size from the scan node, right? Would this also work with more complex queries? Is this "row-size" string only specific for the scan node? 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: ==== > No, the following query runs fine and returns the correct result: Good question. I guess it's just the IN operator that doesn't allow complex types in the following set (inline view in this case), right? I guess it's because for making this work we would need to implement an equality check between complex types to be able to judge if it is member of a set or not. -- 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: 10 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: Thu, 27 Jan 2022 16:24:22 +0000 Gerrit-HasComments: Yes
