Mailing-List: contact reviews-help@impala.incubator.apache.org; run by ezmlm
Precedence: bulk
Message-Id: <201704052109.v35L9woO015688@ip-10-146-233-104.ec2.internal>
Date: Wed, 5 Apr 2017 21:09:58 +0000
From: "Lars Volker (Code Review)" <gerrit@cloudera.org>
To: impala-cr@cloudera.com, reviews@impala.incubator.apache.org
CC: Marcel Kornacker <marcel@cloudera.com>,
        Thomas Tauber-Marshall <tmarshall@cloudera.com>,
        Dimitris Tsirogiannis <dtsirogiannis@cloudera.com>
Reply-To: lv@cloudera.com
Subject: =?UTF-8?Q?=5BImpala-ASF-CR=5D_IMPALA-4166=3A_Add_SORT_BY_sql_clause=0A?=
In-Reply-To: <gerrit.1490644362000.I08834f38a941786ab45a4381c2732d929a934f75@gerrit.cloudera.org>
References: <gerrit.1490644362000.I08834f38a941786ab45a4381c2732d929a934f75@gerrit.cloudera.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Content-Disposition: inline
User-Agent: Gerrit/2.12.7
archived-at: Wed, 05 Apr 2017 21:10:02 -0000

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 <lv@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dtsirogiannis@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <marcel@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tmarshall@cloudera.com>
Gerrit-HasComments: Yes