Mailing-List: contact reviews-help@impala.incubator.apache.org; run by ezmlm
Precedence: bulk
Message-Id: <201704051321.v35DLZk9005058@ip-10-146-233-104.ec2.internal>
Date: Wed, 5 Apr 2017 13:21:35 +0000
From: "Lars Volker (Code Review)" <gerrit@cloudera.org>
To: impala-cr@cloudera.com, reviews@impala.incubator.apache.org
CC: Dimitris Tsirogiannis <dtsirogiannis@cloudera.com>,
        Marcel Kornacker <marcel@cloudera.com>,
        Thomas Tauber-Marshall <tmarshall@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 13:21:41 -0000

Lars Volker has posted comments on this change.

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


Patch Set 8:

(18 comments)

Thank you for the review, please see PS9.

http://gerrit.cloudera.org:8080/#/c/6495/7/fe/src/main/cup/sql-parser.cup
File fe/src/main/cup/sql-parser.cup:

PS7, Line 984: Sor
> There is no KW_SET, right? Update the name?
Done


http://gerrit.cloudera.org:8080/#/c/6495/7/fe/src/main/java/org/apache/impala/analysis/AlterTableSetSortByColumnsStmt.java
File fe/src/main/java/org/apache/impala/analysis/AlterTableSetSortByColumnsStmt.java:

PS7, Line 73: 
> t?
Done


http://gerrit.cloudera.org:8080/#/c/6495/7/fe/src/main/java/org/apache/impala/analysis/Analyzer.java
File fe/src/main/java/org/apache/impala/analysis/Analyzer.java:

Line 2484:    * Add a warning that will be displayed to the user. Ignores null messages. Once
> Comment that no warning can be issued if warnings have been retrieved?
Done


http://gerrit.cloudera.org:8080/#/c/6495/7/fe/src/main/java/org/apache/impala/analysis/CreateTableLikeStmt.java
File fe/src/main/java/org/apache/impala/analysis/CreateTableLikeStmt.java:

PS7, Line 119: ortByColumns_ !
> I think this can throw a NPE. Looking at the parser I see that CreateTableL
Thanks for catching this. I was able to trigger the NPE with an additional test in ToSqlTest.java. Fixed.


http://gerrit.cloudera.org:8080/#/c/6495/7/fe/src/main/java/org/apache/impala/analysis/TableDef.java
File fe/src/main/java/org/apache/impala/analysis/TableDef.java:

PS7, Line 296: 
> inline
Done


http://gerrit.cloudera.org:8080/#/c/6495/7/fe/src/main/java/org/apache/impala/analysis/TablePropertyAnalyzer.java
File fe/src/main/java/org/apache/impala/analysis/TablePropertyAnalyzer.java:

PS7, Line 41: 
            :   /**
            :    * Analyzes the 'sort.by.columns' property in 'tblProperties' against the columns of
            :    * 'table'.
            :    */
            :   public static List<Integer> analyzeSortByColumns(Map<String, String> tblProperties,
            :       Table table) throws AnalysisException {
            :     if (!tblProperties.containsKey(TBL_PROP_SORT_BY_COLUMNS)) {
            :       return Lists.newArrayList();
            :     }
            : 
            :     List<String> sortByCols = Lists.newArrayList(
            :         Splitter.on(",").trimResults().omitEmptyStrings().split(
            :         tblProperties.get(TBL_PROP_SORT_BY_COLUMNS)));
            :     return TablePropertyAnalyzer.analyzeSortByColumns(sortByCols, table);
            :   }
            : 
> These are used in only one place, right? Maybe inline there?
I removed the first one. The second one is used in two places: InsertStmt.analyzeSortByColumns() (that's where I inlined the first one), and AlterTableSetTblProperties.analyze(). I'd keep the second one unless you lean strongly towards inlining it.


Line 76:     }
> Precondition check for HBase?
Done


PS7, Line 127: 
             : 
> You may want to consider if it's better to have a getSortByColumn(Table tab
Inlined it instead.


http://gerrit.cloudera.org:8080/#/c/6495/7/fe/src/main/java/org/apache/impala/planner/Planner.java
File fe/src/main/java/org/apache/impala/planner/Planner.java:

PS7, Line 539: // If the table has a 'sort.by.columns' property and the query has a 'noclustered'
             :     // hint, we issue a warning during analysis and ignore the 'noclustered' hint.
> That comment doesn't seem relevant here. Remove?
I put it there to explain the intent of the if statement. Without it one might think that the noclusteredhint gets ignored by mistake (see previous review comment by Thomas). Should I remove/rephrase it?


PS7, Line 541: nsertStmt.hasClusteredHint() || 
> If insertStmt.hasClusteredHint() is true doesn't that imply that hasNoClust
True, fixed.


http://gerrit.cloudera.org:8080/#/c/6495/7/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java:

PS7, Line 1793: !params.sort_by_columns.isEmpty()
> use isEmpty()
Done


http://gerrit.cloudera.org:8080/#/c/6495/7/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java
File fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java:

Line 526:       String long_property_key = "";
> Test for HBase and Kudu?
Added tests for Kudu in testAlterKuduTable(). HBase tables do not support "ALTER TABLE SET" altogether, which is tested in L683.


Line 1054: 
> Same here (HBase + Kudu). You need to put all Kudu tests in a function and 
Added test to check that HBase is not supported. Added Kudu tests to TestAlterKuduTable().


PS7, Line 1904: te t
> "must" typo
Done


PS7, Line 1920: est
> No need for that comment :)
Done


http://gerrit.cloudera.org:8080/#/c/6495/7/fe/src/test/java/org/apache/impala/analysis/ParserTest.java
File fe/src/test/java/org/apache/impala/analysis/ParserTest.java:

PS7, Line 2347: // Sort by clause
> How about some tests with additional table options? Try changing the order 
Done


http://gerrit.cloudera.org:8080/#/c/6495/7/testdata/workloads/functional-planner/queries/PlannerTest/ddl.test
File testdata/workloads/functional-planner/queries/PlannerTest/ddl.test:

PS7, Line 207: selec
> select query
Done


http://gerrit.cloudera.org:8080/#/c/6495/7/testdata/workloads/functional-planner/queries/PlannerTest/insert-sort-by.test
File testdata/workloads/functional-planner/queries/PlannerTest/insert-sort-by.test:

PS7, Line 194: sort by columns with a join
> can you also add an order by clause?
Done, though it needs a LIMIT, or else the analysis will fail. Now there's a TOP-N node in the plan. Let me know if we also should add a test that ends up having a sort node. Using DISABLE_OUTERMOST_TOPN should allow us to do that.


-- 
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: 8
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