impala-reviews mailing list archives

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

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

Patch Set 10:


Thank you for your reviews. Please see my inline comments and PS10.
File fe/src/main/java/org/apache/impala/analysis/

PS9, Line 376: analyzeSortByColumn
> Instead of checking this here, it's a bit cleaner to alway call analyzeSort
File fe/src/main/java/org/apache/impala/analysis/

PS9, Line 49: roperties.containsKe
> The caller is not supposed to modify the return values, correct? You may ju
Done, I ended up using ImmutableList, since I already use it below.

PS9, Line 77: 
> Same comment as above.

PS9, Line 89: t by column in the li
> You can still construct and return an ImmutableList. See Guava's ImmutableL
I needed the contains() method in line 105, as well as size(), so I ended up using a LinkedHashSet.
File fe/src/test/java/org/apache/impala/analysis/

Line 515:     AnalyzesOk("alter table functional.alltypes set tblproperties(" +
> why allow this in addition to the '... sort by ()' clause?
Alex and I discussed this and Alex suggested that we allow this, since it is easier than explicitly
disabling it, and we'll always have the possibility that someone changes the values in Hive.
The alternative would be to try and hide the table property completely. If Alex doesn't object
and you think it's worth the effort to hide this from the users, I can implement it.

Line 1905:         "tblproperties (''='i')", "Table definition must not contain
" +
> same here, why allow the table property?
Please see my reply above.
File fe/src/test/java/org/apache/impala/analysis/

PS9, Line 2374: // SORT BY must be the first table option
              :     ParserError("CREATE 
> You may want to comment why this results in a parsing error.
File fe/src/test/java/org/apache/impala/analysis/

Line 304:         "TBLPROPERTIES (''='a')", true);
> why not print that as a 'sort by' clause?
I commented on the decision to keep the property visible in
I discussed ToSqlTest() with Alex, too, and IIRC he explained that this is only really relevant
for views, in which create table statements cannot occur anyways. Therefore, this should never
be visible to a user and is tested here only for completeness. Should I add a comment to explain

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I08834f38a941786ab45a4381c2732d929a934f75
Gerrit-PatchSet: 10
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker <>
Gerrit-Reviewer: Dimitris Tsirogiannis <>
Gerrit-Reviewer: Lars Volker <>
Gerrit-Reviewer: Marcel Kornacker <>
Gerrit-Reviewer: Thomas Tauber-Marshall <>
Gerrit-HasComments: Yes

View raw message