impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Thomas Tauber-Marshall (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-4731/IMPALA-397/IMPALA-4728: Materialize sort exprs
Date Thu, 06 Apr 2017 21:31:22 GMT
Thomas Tauber-Marshall has posted comments on this change.

Change subject: IMPALA-4731/IMPALA-397/IMPALA-4728: Materialize sort exprs

Patch Set 3:

File fe/src/main/java/org/apache/impala/analysis/

Line 292:     if (fnName_.isBuiltin()) {
> Sorry for the confusion, let me try to clarify:
File fe/src/main/java/org/apache/impala/analysis/

Line 51:   // Subset of ordering exprs that are materialized. Populated in
> Subset of ordering exprs that are materialized. Populated ...

Line 166:    * output by the sort node. Done by first calling createMaterailzedOrderExprs()
> update comment

Line 170:    * tuple. This simplifies the sorting logic for total and top-n sorts. The substitution
> TODO is addressed

Line 174:       List<Expr> resultExprs, Analyzer analyzer) {
> do these fit into the previous line?

Line 180:     // substOrderBy is a mapping from:
> Comment is confusing because of the original 'sort node's input' which is n

Line 192:     // rematerialization.
> nonMaterializedOrderingExprs

Line 204:         SlotRef cloneRef = new SlotRef(materializedDesc);
> Suggest simplifying comment to something like:

Line 218:   }
> shorter: by creating slots for them in 'sortTupleDesc'

Line 230:       TupleDescriptor sortTupleDesc, Analyzer analyzer) {
> Considering all my comments here and elsewhere I think the simplest interfa

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
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.
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

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: Ifefdaff8557a30ac44ea82ed428e6d1ffbca2e9e
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall <>
Gerrit-Reviewer: Alex Behm <>
Gerrit-Reviewer: Marcel Kornacker <>
Gerrit-Reviewer: Thomas Tauber-Marshall <>
Gerrit-Reviewer: Tim Armstrong <>
Gerrit-HasComments: Yes

View raw message