Alex Behm 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) 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 nois No reason really. Only reason to not do it now is that such renames tend to blow up into larger changes which can make backporting even harder. I've standardized on sortExprs, you can take another look to see if you think it makes sense to make that change in this patch. 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". Done 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. Done 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 in Done 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 Done http://gerrit.cloudera.org:8080/#/c/9631/1/fe/src/main/java/org/apache/impala/analysis/SortInfo.java@212 PS1, Line 212: materializedOrderingExprs_ > stale Done http://gerrit.cloudera.org:8080/#/c/9631/1/fe/src/main/java/org/apache/impala/analysis/SortInfo.java@233 PS1, Line 233: should > must? We chose to materialize some exprs for perf. It's not necessary to materialize them for correctness. 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) Done -- 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: Alex Behm <[email protected]> Gerrit-Reviewer: Thomas Tauber-Marshall <[email protected]> Gerrit-Reviewer: Vuk Ercegovac <[email protected]> Gerrit-Comment-Date: Wed, 14 Mar 2018 19:46:02 +0000 Gerrit-HasComments: Yes
