Thomas Tauber-Marshall has posted comments on this change. Change subject: IMPALA-4731/IMPALA-397/IMPALA-4728: Materialize sort exprs ......................................................................
Patch Set 3: (13 comments) http://gerrit.cloudera.org:8080/#/c/6322/1/fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java File fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java: Line 292: if (fnName_.isBuiltin()) { > Sorry for the confusion, let me try to clarify: Done http://gerrit.cloudera.org:8080/#/c/6322/2/fe/src/main/java/org/apache/impala/analysis/SortInfo.java File fe/src/main/java/org/apache/impala/analysis/SortInfo.java: Line 51: // Subset of ordering exprs that are materialized. Populated in > Subset of ordering exprs that are materialized. Populated ... Done Line 166: * output by the sort node. Done by first calling createMaterailzedOrderExprs() to > update comment Done Line 170: * tuple. This simplifies the sorting logic for total and top-n sorts. The substitution > TODO is addressed Done Line 174: List<Expr> resultExprs, Analyzer analyzer) { > do these fit into the previous line? No. Line 180: // substOrderBy is a mapping from: > Comment is confusing because of the original 'sort node's input' which is n Done Line 192: // rematerialization. > nonMaterializedOrderingExprs Done Line 204: SlotRef cloneRef = new SlotRef(materializedDesc); > Suggest simplifying comment to something like: Done Line 218: } > shorter: by creating slots for them in 'sortTupleDesc' Done Line 230: TupleDescriptor sortTupleDesc, Analyzer analyzer) { > Considering all my comments here and elsewhere I think the simplest interfa Done Line 240: SlotRef materializedRef = new SlotRef(materializedDesc); > Thinking about this again: This behavior complicates the code and function As discussed, I removed the handling of literals. For the inline view - as far as I can tell, the example you gave already works prior to this patch as 'col' gets materialized in a slot. I wasn't quite able to trace how this happens, so perhaps I'm missing something or perhaps there's a more complicated example that doesn't work. http://gerrit.cloudera.org:8080/#/c/6322/2/testdata/workloads/functional-planner/queries/PlannerTest/sort-expr-materialization.test File testdata/workloads/functional-planner/queries/PlannerTest/sort-expr-materialization.test: Line 1: #sort on a non-deterministic expr, gets materialized > Tests should ideally cover the 3 cases: As discussed, the only expr that doesn't have a cost set it subqueries, which can't be used in order bys. http://gerrit.cloudera.org:8080/#/c/6322/1/testdata/workloads/functional-planner/queries/PlannerTest/sort-materialization.test File testdata/workloads/functional-planner/queries/PlannerTest/sort-materialization.test: Line 19 > I think you should be able to add a function using FrontendTestBase.addTest Done -- To view, visit http://gerrit.cloudera.org:8080/6322 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ifefdaff8557a30ac44ea82ed428e6d1ffbca2e9e Gerrit-PatchSet: 3 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Thomas Tauber-Marshall <tmarsh...@cloudera.com> Gerrit-Reviewer: Alex Behm <alex.b...@cloudera.com> Gerrit-Reviewer: Marcel Kornacker <mar...@cloudera.com> Gerrit-Reviewer: Thomas Tauber-Marshall <tmarsh...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-HasComments: Yes
