impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Lars Volker (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-4166: Add SORT BY sql clause
Date Wed, 03 May 2017 21:22:52 GMT
Lars Volker has posted comments on this change.

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


Patch Set 17:

(19 comments)

Thank you for your review! Please see my inline comments and PS 17.

http://gerrit.cloudera.org:8080/#/c/6495/15/fe/src/main/java/org/apache/impala/analysis/AlterTableSetTblProperties.java
File fe/src/main/java/org/apache/impala/analysis/AlterTableSetTblProperties.java:

PS15, Line 168: 
> typo: commas
Done


PS15, Line 169:  '
> remove
Done


PS15, Line 170: f column names separated by commas, 
> nit: this function
Done


PS15, Line 172: during the an
> You need to comment on the return value.
Done


PS15, Line 174:  columns.
> long line
Done


PS15, Line 178: if (!tblProperties.containsKey(
> Whenever I see this, I keep wondering about HBase. Can you add a preconditi
Done


http://gerrit.cloudera.org:8080/#/c/6495/15/fe/src/main/java/org/apache/impala/analysis/AlterTableSortByColumnsStmt.java
File fe/src/main/java/org/apache/impala/analysis/AlterTableSortByColumnsStmt.java:

PS15, Line 44: public static final String TBL_PROP_SORT_BY_COLUMNS = "sort.by.columns";
> Maybe it makes more sense to put this in AlterTableSetTblProperiesStmt. Not
After moving this back and forth, my reasoning was that this is the more natural way to change
the underlying table property, and therefore I left it here. In a similar fashion, the property
name for 'skip.header.line.count' is defined in HdfsTable.java. I also don't have a strong
preference. Let's wait what the next reviewer says then. :)


PS15, Line 93: TableDef.analyzeSortByColumns(sor
> What else could it be? You can just do Preconditions.checkState(table insta
Done


http://gerrit.cloudera.org:8080/#/c/6495/15/fe/src/main/java/org/apache/impala/analysis/InsertStmt.java
File fe/src/main/java/org/apache/impala/analysis/InsertStmt.java:

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


PS15, Line 843:   // 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;
              :         }
              :       }
              :       i
> Not sure I follow what is happening here. If you already have sortByColumns
This code is required for the sortby() hint, and will be removed together with the hint. Should
I try and explain this more in the comment above?


http://gerrit.cloudera.org:8080/#/c/6495/15/fe/src/main/java/org/apache/impala/analysis/TableDef.java
File fe/src/main/java/org/apache/impala/analysis/TableDef.java:

PS15, Line 284: List<Str
> I like using interfaces but is this ever called with anything other than a 
No. I think it was used with the result of a split() in an earlier patchset, but now is only
used for Lists.


PS15, Line 350: if (options_.sortByCols == null) r
> if (options_.sortByCols == null) return;
Done


http://gerrit.cloudera.org:8080/#/c/6495/15/fe/src/main/java/org/apache/impala/analysis/ToSqlUtils.java
File fe/src/main/java/org/apache/impala/analysis/ToSqlUtils.java:

PS15, Line 77: S
> nit: extra space
Done


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.
I re-used the code from L288. Do you have a suggestion how it can be made more clear?


http://gerrit.cloudera.org:8080/#/c/6495/15/fe/src/main/java/org/apache/impala/catalog/Table.java
File fe/src/main/java/org/apache/impala/catalog/Table.java:

PS15, Line 456: he/she adde
> nit: added
I think both ways are fine, adding the columns first, or calling this method first. Changed
it to added though, since that seems to reflect the more common case better.


http://gerrit.cloudera.org:8080/#/c/6495/15/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java:

PS15, Line 1881: String oldColumns = msTbl.getParameters().get(sortByKey);
               :         String alteredColumns = MetaStoreUtil.intersectCsvLis
> I find it hard to parse this. Can you split it into two statements? Also, e
Done


PS15, Line 1915: ring sortByKey = AlterTableSortByColumnsStmt.TBL_PROP_SORT_BY_COLUMNS;
               :         if (msTbl.getParameters().containsKey(sortByKey)) {
> same here
Done


PS15, Line 2198: }
               :     }
> and here
Done


http://gerrit.cloudera.org:8080/#/c/6495/15/fe/src/main/java/org/apache/impala/util/MetaStoreUtil.java
File fe/src/main/java/org/apache/impala/util/MetaStoreUtil.java:

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?
Cool, I didn't know retainAll(). I looked at it, but it looks like I'd have to materialize
both lists to make them lowercase. In the current version I get away with materializing the
right side only. Do you have an idea how to do the transformation and intersection in one
step?


-- 
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: 17
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker <lv@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dtsirogiannis@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <marcel@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tmarshall@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message