Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/17811 )
Change subject: IMPALA-9498: Allow returning arrays in select list ...................................................................... Patch Set 28: (4 comments) Still looking into the FE part. Reviewed some simple changes first. http://gerrit.cloudera.org:8080/#/c/17811/28/fe/src/main/java/org/apache/impala/analysis/SlotRef.java File fe/src/main/java/org/apache/impala/analysis/SlotRef.java: http://gerrit.cloudera.org:8080/#/c/17811/28/fe/src/main/java/org/apache/impala/analysis/SlotRef.java@a105 PS28, Line 105: Could you explain why we should remove this? http://gerrit.cloudera.org:8080/#/c/17811/28/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/17811/28/fe/src/main/java/org/apache/impala/planner/PlanNode.java@161 PS28, Line 161: protected PlanNode(PlanNodeId id, String displayName, : List<CollectionTableRef> tblRefs) { : this(id, displayName); : for (CollectionTableRef collRef : tblRefs) { : tupleIds_.add(collRef.getDesc().getId()); : tblRefIds_.add(collRef.getDesc().getId()); : } : } It seems this is no longer used. I think we can remove it. http://gerrit.cloudera.org:8080/#/c/17811/28/fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java File fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java: http://gerrit.cloudera.org:8080/#/c/17811/28/fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java@322 PS28, Line 322: Preconditions.checkState(!expr.getType().isCollectionType(), : "Sorting is not supported if the select list contains collection columns."); nit: it'd be better to throw AnalysisException instead of IllegalStateException which usually indicates bugs. http://gerrit.cloudera.org:8080/#/c/17811/28/fe/src/main/java/org/apache/impala/planner/UnnestNode.java File fe/src/main/java/org/apache/impala/planner/UnnestNode.java: http://gerrit.cloudera.org:8080/#/c/17811/28/fe/src/main/java/org/apache/impala/planner/UnnestNode.java@63 PS28, Line 63: tupleIds_.addAll(collectionSlotRef.getDesc().getItemTupleDesc().getId().asList()); : tblRefIds_.addAll(collectionSlotRef.getDesc().getItemTupleDesc().getId().asList()); nit: we can add the id directly tupleIds_.add(collectionSlotRef.getDesc().getItemTupleDesc().getId()); tblRefIds_.add(collectionSlotRef.getDesc().getItemTupleDesc().getId()); -- To view, visit http://gerrit.cloudera.org:8080/17811 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ibb1e42ffb21c7ddc033aba0f754b0108e46f34d0 Gerrit-Change-Number: 17811 Gerrit-PatchSet: 28 Gerrit-Owner: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Attila Jeges <[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-Reviewer: Quanlong Huang <[email protected]> Gerrit-Comment-Date: Wed, 15 Dec 2021 12:34:27 +0000 Gerrit-HasComments: Yes
