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 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:

(19 comments)

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: commans
typo: commas


PS15, Line 169: of
remove


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.


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 a strong preference
though.


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.


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 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?


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: 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;


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:  
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.


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 adds
nit: added


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: 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
'get('.


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


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?


-- 
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: 15
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