Lars Volker has posted comments on this change. Change subject: IMPALA-4166: Add SORT BY sql clause ......................................................................
Patch Set 10: (8 comments) Thank you for your reviews. Please see my inline comments and PS10. http://gerrit.cloudera.org:8080/#/c/6495/9/fe/src/main/java/org/apache/impala/analysis/InsertStmt.java File fe/src/main/java/org/apache/impala/analysis/InsertStmt.java: PS9, Line 376: analyzeSortByColumn > Instead of checking this here, it's a bit cleaner to alway call analyzeSort Done 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: 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. Done 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. http://gerrit.cloudera.org:8080/#/c/6495/9/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(" + > 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 ('sort.by.columns'='i')", "Table definition must not contain " + > same here, why allow the sort.by.columns table property? Please see my reply above. http://gerrit.cloudera.org:8080/#/c/6495/9/fe/src/test/java/org/apache/impala/analysis/ParserTest.java File fe/src/test/java/org/apache/impala/analysis/ParserTest.java: 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. Done http://gerrit.cloudera.org:8080/#/c/6495/9/fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java File fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java: Line 304: "TBLPROPERTIES ('sort.by.columns'='a')", true); > why not print that as a 'sort by' clause? I commented on the decision to keep the sort.by.columns property visible in AnalyzeDDLTest.java. 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 this? -- 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: 10 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: Marcel Kornacker <[email protected]> Gerrit-Reviewer: Thomas Tauber-Marshall <[email protected]> Gerrit-HasComments: Yes
