Dimitris Tsirogiannis has posted comments on this change. Change subject: IMPALA-4166: Add SORT BY sql clause ......................................................................
Patch Set 2: (20 comments) Initial pass, haven't looked at the tests yet. http://gerrit.cloudera.org:8080/#/c/6495/2/fe/src/main/cup/sql-parser.cup File fe/src/main/cup/sql-parser.cup: PS2, Line 986: // SORT BY columns are stored in the 'sort.by.columns' table property. : HashMap<String, String> properties = new HashMap<String, String>(); : String columnString = Joiner.on(",").join(col_names); : properties.put(TablePropertyAnalyzer.TBL_PROP_SORT_BY_COLUMNS, columnString); : RESULT = new AlterTableSetTblProperties(table, null, TTablePropertyType.TBL_PROPERTY, : properties); : :} I don't see why you need to treat it as another SetTblProperties statement. I understand that eventually the sort by columns will be stored as table properties but this adds unnecessary complexity to the parser. I would simply create a new AlterTableSetSortByColumns statement. PS2, Line 1143: opt_sort_by_cols:sort_by_cols : KW_LIKE table_name:other_table : opt_comment_val:comment : file_format_create_table_val:file_format location_val:location nit: indentation PS2, Line 1182: opt_sort_by_cols:sort_by_cols opt_comment_val:comment row_format_val:row_format serde_properties:serde_props nit: long line http://gerrit.cloudera.org:8080/#/c/6495/2/fe/src/main/java/org/apache/impala/analysis/AlterTableSetTblProperties.java File fe/src/main/java/org/apache/impala/analysis/AlterTableSetTblProperties.java: PS2, Line 162: /** : * Analyzes the 'sort.by.columns' property to make sure the columns inside the property : * occur in the target table and are neither partitioning nor primary key columns. : */ : public void analyzeSortByColumns() throws AnalysisException { : StringBuilder error = new StringBuilder(); : TablePropertyAnalyzer.analyzeSortByColumns(tblProperties_, getTargetTable(), error); : if (error.length() > 0) throw new AnalysisException(error.toString()); : } Along the lines of my previous comment: a) I would move this completely out of this class, b) create a new AlterTableSetSortByColumns and move the analysis logic there. http://gerrit.cloudera.org:8080/#/c/6495/2/fe/src/main/java/org/apache/impala/analysis/Analyzer.java File fe/src/main/java/org/apache/impala/analysis/Analyzer.java: PS2, Line 275: Boolean boolean? why class? PS2, Line 2486: Preconditions.checkState(!globalState_.warningsRetrieved); This is kind of harsh :). Do we guarantee somewhere that no one will call addWarning after warnings have been retrieved? http://gerrit.cloudera.org:8080/#/c/6495/2/fe/src/main/java/org/apache/impala/analysis/CreateTableLikeStmt.java File fe/src/main/java/org/apache/impala/analysis/CreateTableLikeStmt.java: PS2, Line 119: sortByColumns_.size() > 0 !sortByColumns_.isEmpty() PS2, Line 179: StringBuilder error = new StringBuilder(); : TablePropertyAnalyzer.analyzeSortByColumns(sortByColumns_, srcTable, error); : if (error.length() > 0) throw new AnalysisException(error.toString()); Why don't you just throw the AnalysisException from the analyzeSortByColumns()? http://gerrit.cloudera.org:8080/#/c/6495/2/fe/src/main/java/org/apache/impala/analysis/InsertStmt.java File fe/src/main/java/org/apache/impala/analysis/InsertStmt.java: PS2, Line 382: sortByExprs_.size() > 0 !sortByExprs_.isEmpty() PS2, Line 510: sortByExprs_.add(resultExprs_.get(colIdx)); Does this work ok if you have column permutation? PS2, Line 768: StringBuilder error = new StringBuilder(); : sortByColumns_ = TablePropertyAnalyzer.analyzeSortByColumns(table_, error); : if (error.length() > 0) throw new AnalysisException(error.toString()); Same comment as before. Move the throw clause inside the analyzeSortByColumns() function. http://gerrit.cloudera.org:8080/#/c/6495/2/fe/src/main/java/org/apache/impala/analysis/TablePropertyAnalyzer.java File fe/src/main/java/org/apache/impala/analysis/TablePropertyAnalyzer.java: PS2, Line 33: /** : * This class collects various methods to analyze table properties. : */ : public class TablePropertyAnalyzer { You're only analyzing the sort by columns, so why this generic class? Can't we move these methods to the TableDef class? PS2, Line 41: Analyzes the 'sort.by.columns' property of 'table'. What is the return value? PS2, Line 86: List<String> partitionCols, List<String> primaryKeyCols You can simply pass one list which includes the list of column names to exclude from the sort columns. If you want a specific error message you can construct it in function analyzeSortByColumns() (L62) and pass it as a parameter (which you already do). PS2, Line 90: for (int i = 0; i < sortByCols.size(); ++i) { : String colName = sortByCols.get(i); for (String colName: sortByCols) ? PS2, Line 101: return ImmutableList.<Integer>of(); If I understand correctly, the callers of this function don't do anything with the empty list if the error is populated. Instead, they throw an AnalysisException. So, why simply not throwing the exception here and let the caller handle it if needed? http://gerrit.cloudera.org:8080/#/c/6495/2/fe/src/main/java/org/apache/impala/catalog/Column.java File fe/src/main/java/org/apache/impala/catalog/Column.java: PS2, Line 134: <String> You don't need that. PS2, Line 133: public static List<String> toColumnNames(List<Column> columns) { : List<String> colNames = Lists.<String>newArrayList(); : for (Column col: columns) { colNames.add(col.getName()); } : return colNames; : } Alternate implementation may use the Lists.transform function. http://gerrit.cloudera.org:8080/#/c/6495/2/fe/src/main/java/org/apache/impala/catalog/Table.java File fe/src/main/java/org/apache/impala/catalog/Table.java: PS2, Line 379: public List<String> getColumnNames() { : return Column.toColumnNames(colsByPos_); : } nit: single line? http://gerrit.cloudera.org:8080/#/c/6495/2/fe/src/main/java/org/apache/impala/planner/Planner.java File fe/src/main/java/org/apache/impala/planner/Planner.java: PS2, Line 539: size() > 0 !isEmpty -- 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: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Lars Volker <l...@cloudera.com> Gerrit-Reviewer: Dimitris Tsirogiannis <dtsirogian...@cloudera.com> Gerrit-Reviewer: Lars Volker <l...@cloudera.com> Gerrit-HasComments: Yes
