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 <l...@cloudera.com> Gerrit-Reviewer: Alex Behm <alex.b...@cloudera.com> Gerrit-Reviewer: Dimitris Tsirogiannis <dtsirogian...@cloudera.com> Gerrit-Reviewer: Lars Volker <l...@cloudera.com> Gerrit-Reviewer: Marcel Kornacker <mar...@cloudera.com> Gerrit-Reviewer: Thomas Tauber-Marshall <tmarsh...@cloudera.com> Gerrit-HasComments: Yes
