impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Thomas Tauber-Marshall (Code Review)" <ger...@cloudera.org>
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:

(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 <tmarshall@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <marcel@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tmarshall@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message