impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Lars Volker (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-4166: Add SORT BY sql clause
Date Wed, 10 May 2017 23:45:57 GMT
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 <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