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 Thu, 30 Mar 2017 20:26:13 GMT
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 <lv@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dtsirogiannis@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tmarshall@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message