Lars Volker has posted comments on this change. Change subject: IMPALA-4166: Add SORT BY sql clause ......................................................................
Patch Set 4: (7 comments) Thank you for the reviews. Please see my comments and PS5. http://gerrit.cloudera.org:8080/#/c/6495/3//COMMIT_MSG Commit Message: PS3, Line 9: SORT BY (...) clauses to CREATE : TABLE and ALTER TABLE statements > It would be helpful to have the formal specification of the new syntax here Done PS3, Line 13: TABLE t (i INT, x INT PRIMARY KEY) PARTITION BY HASH(x) : PARTITIONS 8 SORT BY(i) STORED AS KUDU; : CREATE TABLE t SORT BY > Can you expand the comment a little bit on the semantics of this? A few hig Done http://gerrit.cloudera.org:8080/#/c/6495/3/common/thrift/JniCatalog.thrift File common/thrift/JniCatalog.thrift: PS3, Line 382: Optiona > Optional? Done http://gerrit.cloudera.org:8080/#/c/6495/3/fe/src/main/cup/sql-parser.cup File fe/src/main/cup/sql-parser.cup: PS3, Line 1132: // This extra production is necessary since without it the parser will not be able to : // parse "CREATE TABLE A LIKE B". > I don't understand this comment, since the sort_col_cols here is optional. At first I tried adding sort_by_cols to the production above, but then the parser would not pick it up for queries like "CREATE TABLE A LIKE B" and instead try to parse it only as a "CREATE TABLE LIKE FILE" query. I could not figure out why it does that, but copying the rule fixes it. I agree that it should work. I'll ask Alex for help. http://gerrit.cloudera.org:8080/#/c/6495/3/fe/src/main/java/org/apache/impala/catalog/Table.java File fe/src/main/java/org/apache/impala/catalog/Table.java: Line 452: > I think this only makes sense in the context of testing? If so, note that h Done http://gerrit.cloudera.org:8080/#/c/6495/3/fe/src/main/java/org/apache/impala/planner/Planner.java File fe/src/main/java/org/apache/impala/planner/Planner.java: PS3, Line 539: s' property and the query has a 'noclu > I think that you need a '&& !hasNoClusteredHint()' here. Done, also added a comment to explain that the hint will be ignored if there are sort by columns. http://gerrit.cloudera.org:8080/#/c/6495/3/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java File fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java: Line 515: AnalyzesOk("alter table functional.alltypes set tblproperties(" + > Is 'sort.by.columns'='' handled? Yes, added a test. -- 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: 4 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Lars Volker <[email protected]> Gerrit-Reviewer: Dimitris Tsirogiannis <[email protected]> Gerrit-Reviewer: Lars Volker <[email protected]> Gerrit-Reviewer: Thomas Tauber-Marshall <[email protected]> Gerrit-HasComments: Yes
