Taras Bobrovytsky has posted comments on this change. Change subject: IMPALA-4731/IMPALA-397/IMPALA-4728: Materialize sort exprs ......................................................................
Patch Set 4: (3 comments) http://gerrit.cloudera.org:8080/#/c/5914/4/common/thrift/ImpalaInternalService.thrift File common/thrift/ImpalaInternalService.thrift: PS4, Line 226: materialize_sort Right now this query options can have 3 values: true, false, not set and each one leads to a different behavior. I think that's confusing. I would rename this to "always_materialize_sort" and set it to false by default. http://gerrit.cloudera.org:8080/#/c/5914/4/fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java File fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java: Line 272: public boolean computeIsDeterministic() { If the function name starts with "compute" that implies to me that it will compute the value and store it somewhere. Instead, this value is returned. Maybe rename it to simply isDeterministic() http://gerrit.cloudera.org:8080/#/c/5914/4/fe/src/main/java/org/apache/impala/analysis/SortInfo.java File fe/src/main/java/org/apache/impala/analysis/SortInfo.java: Line 227: return new ExprSubstitutionMap(); Why not let it print all nondeterministic expressions? i.e. remove the return here. -- 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: 4 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: Impala Public Jenkins Gerrit-Reviewer: Taras Bobrovytsky <tbobrovyt...@cloudera.com> Gerrit-Reviewer: Thomas Tauber-Marshall <tmarsh...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-HasComments: Yes
