Lars Volker has posted comments on this change. Change subject: IMPALA-4734: Set parquet::RowGroup::sorting_columns ......................................................................
Patch Set 1: (6 comments) Thank you for the review. Please see my comments and PS2. http://gerrit.cloudera.org:8080/#/c/6219/1/be/src/exec/hdfs-table-sink.h File be/src/exec/hdfs-table-sink.h: Line 268: // Stores the positions of columns in the target table that are mentioned in the > 'positions' is vague Done http://gerrit.cloudera.org:8080/#/c/6219/1/common/thrift/DataSinks.thrift File common/thrift/DataSinks.thrift: Line 74: // Stores the positions of columns in the target table that are mentioned in the > positions sounds like indices. can you point out exactly into what they ind Done http://gerrit.cloudera.org:8080/#/c/6219/1/fe/src/main/java/org/apache/impala/analysis/UpdateStmt.java File fe/src/main/java/org/apache/impala/analysis/UpdateStmt.java: Line 69: ImmutableList.<Integer>of()); > i'd rather pass a null here The comment on TableSink.create() required all other lists to be non-null and uses empty lists when needed. If you don't feel strongly about this, I'd prefer to keep that intact. Let me know if you'd like me to change it. http://gerrit.cloudera.org:8080/#/c/6219/1/fe/src/main/java/org/apache/impala/planner/Planner.java File fe/src/main/java/org/apache/impala/planner/Planner.java: Line 551: List<Boolean> isAscOrder = Collections.nCopies(orderingExprs.size(), true); > also mention in commit msg that you're changing sort direction It's already there. Should I make it more prominent? http://gerrit.cloudera.org:8080/#/c/6219/1/fe/src/main/java/org/apache/impala/planner/TableSink.java File fe/src/main/java/org/apache/impala/planner/TableSink.java: Line 89: * don't make sense for a certain table type. > comment on new parameter (can it be null, etc.) Commented on the parameter. I'd prefer to keep it non-null like the others. Let me know if you prefer to change that aspect. Line 94: if (table instanceof HdfsTable) { > for the other table types, you should check that sortbycols isn't set Done -- To view, visit http://gerrit.cloudera.org:8080/6219 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ib42aab585e9e627796e9510e783652d49d74b56c Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Lars Volker <[email protected]> Gerrit-Reviewer: Lars Volker <[email protected]> Gerrit-Reviewer: Marcel Kornacker <[email protected]> Gerrit-HasComments: Yes
