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 <l...@cloudera.com> Gerrit-Reviewer: Alex Behm <alex.b...@cloudera.com> Gerrit-Reviewer: Dimitris Tsirogiannis <dtsirogian...@cloudera.com> Gerrit-Reviewer: Lars Volker <l...@cloudera.com> Gerrit-Reviewer: Marcel Kornacker <mar...@cloudera.com> Gerrit-Reviewer: Thomas Tauber-Marshall <tmarsh...@cloudera.com> Gerrit-HasComments: Yes
