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-4734: Set parquet::RowGroup::sorting columns
Date Mon, 06 Mar 2017 19:50:48 GMT
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 <lv@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <marcel@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message