impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Dimitris Tsirogiannis (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-4166: Add SORT BY sql clause
Date Tue, 28 Mar 2017 22:34:10 GMT
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 <lv@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dtsirogiannis@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message