impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Dimitris Tsirogiannis (Code Review)" <>
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:


Initial pass, haven't looked at the tests yet.
File fe/src/main/cup/sql-parser.cup:

PS2, Line 986: // SORT BY columns are stored in the '' 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
nit: long line
File fe/src/main/java/org/apache/impala/analysis/

PS2, Line 162: /**
             :    * Analyzes the '' property to make sure the columns inside
the property
             :    * occur in the target table and are neither partitioning nor primary key
             :    */
             :   public void analyzeSortByColumns() throws AnalysisException {
             :     StringBuilder error = new StringBuilder();
             :     TablePropertyAnalyzer.analyzeSortByColumns(tblProperties_, getTargetTable(),
             :     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.
File fe/src/main/java/org/apache/impala/analysis/

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?
File fe/src/main/java/org/apache/impala/analysis/

PS2, Line 119: sortByColumns_.size() > 0

PS2, Line 179: StringBuilder error = new StringBuilder();
             :       TablePropertyAnalyzer.analyzeSortByColumns(sortByColumns_, srcTable,
             :       if (error.length() > 0) throw new AnalysisException(error.toString());
Why don't you just throw the AnalysisException from the analyzeSortByColumns()?
File fe/src/main/java/org/apache/impala/analysis/

PS2, Line 382:  sortByExprs_.size() > 0

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.
File fe/src/main/java/org/apache/impala/analysis/

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 '' 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?
File fe/src/main/java/org/apache/impala/catalog/

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.
File fe/src/main/java/org/apache/impala/catalog/

PS2, Line 379: public List<String> getColumnNames() {
             :     return Column.toColumnNames(colsByPos_);
             :   }
nit: single line?
File fe/src/main/java/org/apache/impala/planner/

PS2, Line 539: size() > 0

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I08834f38a941786ab45a4381c2732d929a934f75
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker <>
Gerrit-Reviewer: Dimitris Tsirogiannis <>
Gerrit-Reviewer: Lars Volker <>
Gerrit-HasComments: Yes

View raw message