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 <l...@cloudera.com> Gerrit-Reviewer: Alex Behm <alex.b...@cloudera.com> Gerrit-Reviewer: Dimitris Tsirogiannis <dtsirogian...@cloudera.com> Gerrit-Reviewer: Lars Volker <l...@cloudera.com> Gerrit-Reviewer: Marcel Kornacker <mar...@cloudera.com> Gerrit-Reviewer: Thomas Tauber-Marshall <tmarsh...@cloudera.com> Gerrit-HasComments: Yes
