Thomas Tauber-Marshall has posted comments on this change. Change subject: IMPALA-4166: Add SORT BY sql clause ......................................................................
Patch Set 3: (6 comments) Just a heads up, this is going to have some conflicts with IMPALA-3742 (partition and sort Kudu inserts) which I'll have a patch out for soon, but this will probably go in first so I guess that's my problem. 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, to show where the clause would go, e.g.: ALTER TABLE name SORT BY (col_spec[, col_spec...]) etc. http://gerrit.cloudera.org:8080/#/c/6495/3/common/thrift/JniCatalog.thrift File common/thrift/JniCatalog.thrift: PS3, Line 382: Options Optional? 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. 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: public void setNumClusteringCols(int n) { numClusteringCols_ = n; } I think this only makes sense in the context of testing? If so, note that here. 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: !insertStmt.getSortByExprs().isEmpty() I think that you need a '&& !hasNoClusteredHint()' here. 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: AnalysisError("alter table functional.alltypes set tblproperties(" + Is 'sort.by.columns'='' handled? -- 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: 3 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
