impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Marcel Kornacker (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-4166: Add SORT BY sql clause
Date Wed, 10 May 2017 19:46:45 GMT
Marcel Kornacker has posted comments on this change.

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


Patch Set 22:

(13 comments)

http://gerrit.cloudera.org:8080/#/c/6495/22/common/thrift/JniCatalog.thrift
File common/thrift/JniCatalog.thrift:

Line 382:   // Optional list of sort by columns for the new table. If specified, these will
override
"sort columns" or "sorting columns" (as in: partition columns)


Line 385:   9: optional list<string> sort_by_columns
sort_columns


Line 439:   16: optional list<string> sort_by_columns
same


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

Line 336: nonterminal ArrayList<String> opt_ident_list, opt_sort_by_cols;
opt_sort_by_clause or opt_sort_cols? "sort by columns" sounds odd.


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

Line 43: public class AlterTableSortByColumnsStmt extends AlterTableStmt {
drop "columns", the statement doesn't include a COLUMNS


Line 73:     // Disallow setting sort by columns on HBase and Kudu tables.
rephrase "sort by columns" as "sort columns" universally


Line 91:   public static void analyzeSortByColumns(List<String> sortByCols, Table table)
maybe move this into TableDef as well? it seems kind of arbitrary to put it here.


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

Line 863:         // TODO: Remove this when removing the sortby() hint (IMPALA-5157).
is this going to happen soon?


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

Line 46:  * correspond to the following clauses in a CREATE TABLE statement:
update


Line 343:     final String sortByKey = AlterTableSortByColumnsStmt.TBL_PROP_SORT_BY_COLUMNS;
why final?


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

Line 75:     final String sortByKey = AlterTableSortByColumnsStmt.TBL_PROP_SORT_BY_COLUMNS;
it feels like that constant should live somewhere else


http://gerrit.cloudera.org:8080/#/c/6495/22/fe/src/main/java/org/apache/impala/catalog/Table.java
File fe/src/main/java/org/apache/impala/catalog/Table.java:

Line 456:    * the caller must make sure that the value matches any columns he/she added to
the
or "that were added to the table" to avoid the gender reference :)


http://gerrit.cloudera.org:8080/#/c/6495/22/fe/src/test/java/org/apache/impala/planner/PlannerTest.java
File fe/src/test/java/org/apache/impala/planner/PlannerTest.java:

Line 101:     addTestTable("create table test_sort_by.alltypes (id int, int_col int, " +
why call this alltypes? it clearly doesn't contain all types.


-- 
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: 22
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker <lv@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@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

Mime
View raw message