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 Mon, 17 Apr 2017 22:15:37 GMT
Dimitris Tsirogiannis has posted comments on this change.

Change subject: IMPALA-4166: Add SORT BY sql clause

Patch Set 15:

File fe/src/main/java/org/apache/impala/analysis/

PS15, Line 168: commans
typo: commas

PS15, Line 169: of

PS15, Line 170: this will throw an AnalysisException
nit: this function

PS15, Line 172: List<Integer>
You need to comment on the return value.

PS15, Line 174: if (!tblProperties.containsKey(AlterTableSortByColumnsStmt.TBL_PROP_SORT_BY_COLUMNS))
long line

PS15, Line 178: if (table instanceof KuduTable) {
Whenever I see this, I keep wondering about HBase. Can you add a precondition check or a similar
check? whatever is applicable.
File fe/src/main/java/org/apache/impala/analysis/

PS15, Line 44: public static final String TBL_PROP_SORT_BY_COLUMNS = "";
Maybe it makes more sense to put this in AlterTableSetTblProperiesStmt. Not a strong preference

PS15, Line 93: if (table instanceof HdfsTable) {
What else could it be? You can just do Preconditions.checkState(table instanceof HdfsTable);
and then cast and remove the if statement.
File fe/src/main/java/org/apache/impala/analysis/

PS15, Line 768: if (table_ instanceof HBaseTable) return;
              :     if (table_ instanceof KuduTable) return;
Why not if (!(table_ instanceof HdfsTable)) return; instead?

PS15, Line 843: if (!columns.isEmpty()) {
              :         // We need to make a copy to make the sortByColumns_ list mutable.
              :         // TODO: Remove this when removing the sortby() hint (IMPALA-5157).
              :         sortByColumns_ = Lists.newArrayList(sortByColumns_);
              :       }
              :       for (int i = 0; i < columns.size(); ++i) {
              :         if (columns.get(i).getName().equals(columnName)) {
              :           sortByExprs_.add(resultExprs_.get(i));
              :           sortByColumns_.add(i);
              :           foundColumn = true;
              :           break;
              :         }
              :       }
Not sure I follow what is happening here. If you already have sortByColumns_ do you expand
them based on the hint or something else? Do we have tests for that?
File fe/src/main/java/org/apache/impala/analysis/

PS15, Line 284: Iterable
I like using interfaces but is this ever called with anything other than a List?

PS15, Line 350: if (options_.sortByCols != null) {
if (options_.sortByCols == null) return;
File fe/src/main/java/org/apache/impala/analysis/

PS15, Line 77:  
nit: extra space

PS15, Line 297: if (sortByColsSql != null) {
              :       sb.append(String.format("SORT BY (\n  %s\n)\n",
              :           Joiner.on(", \n  ").join(sortByColsSql)));
              :     }
haha, I can't visualize the output here.
File fe/src/main/java/org/apache/impala/catalog/

PS15, Line 456: he/she adds
nit: added
File fe/src/main/java/org/apache/impala/service/

PS15, Line 1881: msTbl.getParameters().put(sortByKey, MetaStoreUtil.intersectCsvListWithColumNames(
               :             msTbl.getParameters().get( sortByKey), columns));
I find it hard to parse this. Can you split it into two statements? Also, extra space after

PS15, Line 1915: msTbl.getParameters().put(sortByKey, MetaStoreUtil.replaceValueInCsvList(
               :               msTbl.getParameters().get( sortByKey), colName, newCol.getColumnName()));
same here

PS15, Line 2198: msTbl.getParameters().put(sortByKey, MetaStoreUtil.removeValueFromCsvList(
               :           msTbl.getParameters().get( sortByKey), colName));
and here
File fe/src/main/java/org/apache/impala/util/

PS15, Line 218: HashSet<String> rightColNames = Sets.newHashSet();
              :     for (TColumn c : rightCols) rightColNames.add(c.getColumnName().toLowerCase());
              :     List<String> outputList = Lists.newArrayList();
              :     for (String leftCol : leftCols) {
              :       if (rightColNames.contains(leftCol.toLowerCase())) outputList.add(leftCol);
              :     }
Can't you use Set.retainAll() here?

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I08834f38a941786ab45a4381c2732d929a934f75
Gerrit-PatchSet: 15
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker <>
Gerrit-Reviewer: Alex Behm <>
Gerrit-Reviewer: Dimitris Tsirogiannis <>
Gerrit-Reviewer: Lars Volker <>
Gerrit-Reviewer: Marcel Kornacker <>
Gerrit-Reviewer: Thomas Tauber-Marshall <>
Gerrit-HasComments: Yes

View raw message