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 <tmarsh...@cloudera.com> Gerrit-Reviewer: Alex Behm <alex.b...@cloudera.com> Gerrit-Reviewer: Thomas Tauber-Marshall <tmarsh...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-HasComments: Yes
