impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Alex Behm (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-4731: Crash when sorting on non-deterministic expr
Date Wed, 08 Feb 2017 23:37:18 GMT
Alex Behm has posted comments on this change.

Change subject: IMPALA-4731: Crash when sorting on non-deterministic expr
......................................................................


Patch Set 2:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/5914/2/fe/src/main/java/org/apache/impala/analysis/SortInfo.java
File fe/src/main/java/org/apache/impala/analysis/SortInfo.java:

Line 173:     ExprSubstitutionMap substOrderBy = new ExprSubstitutionMap();
It's important to update this smap according to the new materialization you are doing below.
I'd prefer to keep the original flow where we populate this smap and then substituteOrderingExprs().

For example, one case that I believe will not work as expected is:

select rand() r from t order by r

The reason is that we will not substitute the rand() from the select list with the materialized
slot produced in the order by - but we should. Similar arguments apply for something like:

select my_expensive_expr() e from t order by e

We will redundantly evaluate my_expensive_expr in the select list again.


Line 186:     // TODO: it may be better for performance to not materialize some exprs that
are
I think we should address this TODO while we are here.

We should materialize:
1. All known non-deterministic functions.
We can add an Expr.isDeterministic() function and implement that for FunctionCallExpr like
we implement isConstant().

2. All UDFs. We can check whether an Expr is a UDF.

3. Exprs that are 'expensive' according to some cost threshold. If the cost is unknown, I'd
say materialize.


-- 
To view, visit http://gerrit.cloudera.org:8080/5914
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I5dcda32fc7770d42fc500ce87fc54d58e5b5dc00
Gerrit-PatchSet: 2
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: Thomas Tauber-Marshall <tmarshall@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message