Dimitris Tsirogiannis has posted comments on this change. Change subject: IMPALA-3742: Partitions and sort INSERTs for Kudu tables ......................................................................
Patch Set 2: (6 comments) Quick pass with some minor comments. I am still trying to wrap my head around the concept of KuduPartitionExpr... http://gerrit.cloudera.org:8080/#/c/6559/1/be/src/exec/kudu-util.h File be/src/exec/kudu-util.h: PS1, Line 62: int col, PrimitiveType type Comment on these params. http://gerrit.cloudera.org:8080/#/c/6559/1/be/src/runtime/coordinator.cc File be/src/runtime/coordinator.cc: PS1, Line 1845: f (fragment.__isset.output_sink && fragment.output_sink.__isset.stream_sink : && fragment.output_sink.type == TDataSinkType::DATA_STREAM_SINK : && fragment.output_sink.stream_sink.output_partition.type == TPartitionType::KUDU) Add a brief comment. http://gerrit.cloudera.org:8080/#/c/6559/1/common/thrift/Exprs.thrift File common/thrift/Exprs.thrift: PS1, Line 134: 4 2? PS1, Line 137: 5 3? :) http://gerrit.cloudera.org:8080/#/c/6559/1/fe/src/main/java/org/apache/impala/analysis/InsertStmt.java File fe/src/main/java/org/apache/impala/analysis/InsertStmt.java: PS1, Line 113: // Set in analyze(). Maps exprs in partitionKeyExprs_ to their column's position in the : // table, eg. partitionKeyExprs_[i] = table_.columns(partitionKeyIdx_[i]) : private List<Integer> partitionKeyIdxs_ = Lists.newArrayList(); Just for my understanding, can you explain why this is needed for this change? http://gerrit.cloudera.org:8080/#/c/6559/1/fe/src/main/java/org/apache/impala/planner/DataPartition.java File fe/src/main/java/org/apache/impala/planner/DataPartition.java: PS1, Line 48: // For kudu partition: expr that calls into the kudu client to get the partition number. : private Expr partitionExpr_; : : // Should be called only by the static factory method for Kudu partitioned tables. : private DataPartition(TPartitionType type, Expr partitionExpr) { : Preconditions.checkNotNull(partitionExpr); : Preconditions.checkState(type == TPartitionType.KUDU); : type_ = type; : partitionExpr_ = partitionExpr; : partitionExprs_ = Lists.newArrayList(); : } I am not sure I get this. Why not passing a single element list of the case of Kudu partitioned? -- To view, visit http://gerrit.cloudera.org:8080/6559 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I84ce0032a1b10958fdf31faef225372c5c38fdc4 Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Thomas Tauber-Marshall <tmarsh...@cloudera.com> Gerrit-Reviewer: Dimitris Tsirogiannis <dtsirogian...@cloudera.com> Gerrit-Reviewer: Matthew Jacobs <m...@cloudera.com> Gerrit-HasComments: Yes
