Gabor Kaszab has posted comments on this change. ( http://gerrit.cloudera.org:8080/18094 )
Change subject: IMPALA-11038: Zipping unnest from view ...................................................................... Patch Set 8: (5 comments) http://gerrit.cloudera.org:8080/#/c/18094/7/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/18094/7/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@510 PS7, Line 510: If the table ref is originated from a view then also add the tuple IDs for the : // respective table refs from the view. > Is this still true in the last patch? I think this is still true as e.g. for a from clause unnest I still had to store the zipping unnest IDs both in the FromClause and in CollectionTableRef.analyze(). For the select list syntax again we store the zipping unnest IDs in the FromClause (in the re-analysis phase) and in UnnestExpr.analyze(). See the calsites for addZippingUnnestTupleId() http://gerrit.cloudera.org:8080/#/c/18094/7/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@515 PS7, Line 515: public int numZippingUnnests = 0; > Do we still need this? I think that zippingUnnestTupleIds's length should b see comment above. http://gerrit.cloudera.org:8080/#/c/18094/7/fe/src/main/java/org/apache/impala/analysis/CollectionTableRef.java File fe/src/main/java/org/apache/impala/analysis/CollectionTableRef.java: http://gerrit.cloudera.org:8080/#/c/18094/7/fe/src/main/java/org/apache/impala/analysis/CollectionTableRef.java@206 PS7, Line 206: isCollectionInSelectList > I couldn't find where we use this. I use it twice in UnnestExpr. http://gerrit.cloudera.org:8080/#/c/18094/7/fe/src/main/java/org/apache/impala/analysis/FromClause.java File fe/src/main/java/org/apache/impala/analysis/FromClause.java: http://gerrit.cloudera.org:8080/#/c/18094/7/fe/src/main/java/org/apache/impala/analysis/FromClause.java@150 PS7, Line 150: // Don't do any checks of the collection that came from a view as getTable() would : // return null in that case. : if (collRef.getTable() == null) return; > Shouldn't line 149 be enough to rule out removing this check would crash this query: select id, a1, a2 from ( select id, unnest(arr1) a1, unnest(arr2) a2 from complextypes_arrays where id = 3 or id = 4) x where a1 > 8 and a2 = 'ten'; http://gerrit.cloudera.org:8080/#/c/18094/7/fe/src/main/java/org/apache/impala/planner/PlanNode.java File fe/src/main/java/org/apache/impala/planner/PlanNode.java: http://gerrit.cloudera.org:8080/#/c/18094/7/fe/src/main/java/org/apache/impala/planner/PlanNode.java@506 PS7, Line 506: removeZippingUnnestConjuncts > Is this still needed? What I don't get is why is it needed now, but why it hmm, it was a long time ago when I wrote this ... :D I did some poking here and it seems that for some reason the SingularRowSrcNode picks up zipping unnest conjuncts (and fails on a Precondition as it shouldn't) instead of the UnnestNode. Also, if I remove this function the "select list unnest" tests also break. Currently, I don't recall why this is needed for unnesting from views but not for tables. I'll take a look to figure out. Update: Ok, I figured it out why this is needed. In case there are conjuncts on an unnested array where the array is coming from a view the UnnestNode originally (and none of the other nodes) didn't pick up these conjuncts and they weren't evaluated. I had to make the changes on isBoundByTupleIds() functions in UnnestExpr and SlotRef but then the ScanNode also wanted to pick up these, also the SingularRowSrcNode so this is what I came up with to prevent them to pick up these unnested array related conjuncts and let UnnestNode to take care of them. The above only applies if there are more than 1 of these unnest conjuncts. If there is one, than ScanNode handles it. -- To view, visit http://gerrit.cloudera.org:8080/18094 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I68f649dda9e41f257e7f6596193d07b24049f92a Gerrit-Change-Number: 18094 Gerrit-PatchSet: 8 Gerrit-Owner: Gabor Kaszab <[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: Wed, 30 Mar 2022 09:15:16 +0000 Gerrit-HasComments: Yes
