Lars Volker has posted comments on this change. Change subject: IMPALA-4166: Add SORT BY sql clause ......................................................................
Patch Set 23: (10 comments) Thanks for the review, please see PS24. 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_cols; > opt_sort_by_clause or opt_sort_cols? "sort by columns" sounds odd. Done 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 > drop "columns", the statement doesn't include a COLUMNS Done Line 73 > rephrase "sort by columns" as "sort columns" universally Done Line 91 > maybe move this into TableDef as well? it seems kind of arbitrary to put it Done. This has moved a couple of times now. :) 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? Yes, immediately after this change makes it in. We should remove the hint before releasing 2.9, since it hasn't been released yet. We could remove it in this change at the risk of the change becoming even larger than it already is. 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: > update Done Line 343: if (options_.location != null) { > why final? To express that it's just a shorter name for the constant. Removed it. 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 = AlterTableSortByStmt.TBL_PROP_SORT_COLUMNS; > it feels like that constant should live somewhere else I've discussed that with Alex and Dimitris and we no better place has emerged, though I've tried some of them. It's related to the sort by clause, similar to how TBL_PROP_SKIP_HEADER_LINE_COUNT lives in HdfsTable.java. Do you have a preference where it should go? I also thought of TableDef (where the analysis happens) or AlterTableSetTblProperties (where we deal with properties). 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 that were added to the > or "that were added to the table" to avoid the gender reference :) Done 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.t (id int, int_col int, " + > why call this alltypes? it clearly doesn't contain all types. Done -- 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: 23 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
