Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/9631 )
Change subject: IMPALA-5270: Pass resolved exprs into analytic SortInfo. ...................................................................... Patch Set 1: (8 comments) several comments while skimming the change. looks fine (and glad to see SortInfo getting cleaned up) http://gerrit.cloudera.org:8080/#/c/9631/1/fe/src/main/java/org/apache/impala/analysis/SortInfo.java File fe/src/main/java/org/apache/impala/analysis/SortInfo.java: http://gerrit.cloudera.org:8080/#/c/9631/1/fe/src/main/java/org/apache/impala/analysis/SortInfo.java@47 PS1, Line 47: ordering any reason to distinguish between 'sort' and 'order' (might reduce the noise to settle on one)? http://gerrit.cloudera.org:8080/#/c/9631/1/fe/src/main/java/org/apache/impala/analysis/SortInfo.java@51 PS1, Line 51: sorted, and slightly puzzled by the implication here: "sorting a single tuple". http://gerrit.cloudera.org:8080/#/c/9631/1/fe/src/main/java/org/apache/impala/analysis/SortInfo.java@106 PS1, Line 106: sortTupleSlotExpr nit: stale. http://gerrit.cloudera.org:8080/#/c/9631/1/fe/src/main/java/org/apache/impala/analysis/SortInfo.java@169 PS1, Line 169: ns.checkState(sortTupleDesc while here, perhaps clarify the intended sequence of method calls. I can infer that this method must go first, followed by the materializeRequiredSlots method, but that's just by observing preconditions. http://gerrit.cloudera.org:8080/#/c/9631/1/fe/src/main/java/org/apache/impala/analysis/SortInfo.java@209 PS1, Line 209: given dup, remove http://gerrit.cloudera.org:8080/#/c/9631/1/fe/src/main/java/org/apache/impala/analysis/SortInfo.java@212 PS1, Line 212: materializedOrderingExprs_ stale http://gerrit.cloudera.org:8080/#/c/9631/1/fe/src/main/java/org/apache/impala/analysis/SortInfo.java@233 PS1, Line 233: should must? http://gerrit.cloudera.org:8080/#/c/9631/1/fe/src/main/java/org/apache/impala/planner/SortNode.java File fe/src/main/java/org/apache/impala/planner/SortNode.java: http://gerrit.cloudera.org:8080/#/c/9631/1/fe/src/main/java/org/apache/impala/planner/SortNode.java@135 PS1, Line 135: List<SlotDescriptor> sortTupleSlots = info_.getSortTupleDescriptor().getSlots(); : List<Expr> slotExprs = info_.getMaterializedExprs(); : P move this invariant into SortInfo (perhaps for another change) -- To view, visit http://gerrit.cloudera.org:8080/9631 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id2b3f4e5e3f1fd441a63160db3c703c432fbb072 Gerrit-Change-Number: 9631 Gerrit-PatchSet: 1 Gerrit-Owner: Alex Behm <[email protected]> Gerrit-Reviewer: Thomas Tauber-Marshall <[email protected]> Gerrit-Reviewer: Vuk Ercegovac <[email protected]> Gerrit-Comment-Date: Wed, 14 Mar 2018 18:05:05 +0000 Gerrit-HasComments: Yes
