Lars Volker has posted comments on this change. Change subject: IMPALA-4166: Add SORT BY sql clause ......................................................................
Patch Set 25: (4 comments) Addressed the final comments, will rebase next. 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 295: > when you're output error messages, it's probably better to refer to the exa Done Line 352: "Please retry without caching: CREATE TABLE ... UNCACHED", > why not "SORT BY is not supported..."? Done http://gerrit.cloudera.org:8080/#/c/6495/25/fe/src/main/java/org/apache/impala/analysis/TableDef.java File fe/src/main/java/org/apache/impala/analysis/TableDef.java: Line 291: TableDef.analyzeSortColumns(sortCols, > do you need the 'tabledef.' prefix here? No, 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: String sortByKey = AlterTableSortByStmt.TBL_PROP_SORT_COLUMNS; > aren't you implying that it should live in hdfstable as well? No, I meant to say that the skip.header.line.count key constant lives where it semantically somewhat belongs to, as compared to AlterTableSetTblProperties or TableDef. The class that most exclusively deals with SORT BY is this one here. Apologies for the confusion. :) -- 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: 25 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
