impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Lars Volker (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-4166: Add SORT BY sql clause
Date Sun, 09 Apr 2017 13:13:24 GMT
Lars Volker has posted comments on this change.

Change subject: IMPALA-4166: Add SORT BY sql clause
......................................................................


Patch Set 12:

(5 comments)

Thank you Alex for the review. Please see my inline comments and PS13.

http://gerrit.cloudera.org:8080/#/c/6495/9/fe/src/main/java/org/apache/impala/analysis/TablePropertyAnalyzer.java
File fe/src/main/java/org/apache/impala/analysis/TablePropertyAnalyzer.java:

Line 85:    */
Each of the public methods is called at least twice from elsewhere so inlining them seems
wasteful. For cases where we don't have a table yet (e.g. create table statements), I can't
think of an interface that's much better than this. Should we build lookup tables for the
column lists first to address your performance considerations? We could also try build an
interface and dummy table for cases where we don't have a table yet.

Should we discuss this in a short meeting?


Line 114:       }
> Does this mean we need 2 ALTER statements to drop/rename a column?
Yes, currently that's the case. I think we discussed this in person once, too, but I don't
remember what we decided to do. Do you have a preference? Should the drop/rename statement
fail if the columns is still referenced in the sort by list? Or at least issue a warning?


Line 115:       if (!foundColumn) {
Yes, in AnalyzeDDLStmts.java.


http://gerrit.cloudera.org:8080/#/c/6495/9/fe/src/main/java/org/apache/impala/catalog/Table.java
File fe/src/main/java/org/apache/impala/catalog/Table.java:

Line 458:    */
> Preconditions.checkStats(RuntimeEnv.isTestEnv());
Cool, done.


http://gerrit.cloudera.org:8080/#/c/6495/9/testdata/workloads/functional-planner/queries/PlannerTest/insert-sort-by.test
File testdata/workloads/functional-planner/queries/PlannerTest/insert-sort-by.test:

Line 170: # IMPALA-4166: sort by columns are correct when using a partial column permutation.
> What if neither bool_col or int_col are mentioned in the column permutation
No, constant exprs are removed from the sortby list in the Planner, and for an empty list
we don't create a sort node at all. I added a test to make sure this works as intended.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I08834f38a941786ab45a4381c2732d929a934f75
Gerrit-PatchSet: 12
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker <lv@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dtsirogiannis@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <marcel@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tmarshall@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message