impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Dan Hecht (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-3567 Part 2, IMPALA-3899: factor out PHJ builder
Date Wed, 21 Sep 2016 22:09:46 GMT
Dan Hecht has posted comments on this change.

Change subject: IMPALA-3567 Part 2, IMPALA-3899: factor out PHJ builder
......................................................................


Patch Set 18:

(18 comments)

http://gerrit.cloudera.org:8080/#/c/3873/18/be/src/exec/partitioned-hash-join-builder.cc
File be/src/exec/partitioned-hash-join-builder.cc:

PS18, Line 326: we'd like all partitions to
... all partitions ..


PS18, Line 327: be
are 
(to match above edit)


Line 353:   // building hash tables because allocating probe buffers may cause some more partitions
duplicated sentences


PS18, Line 797: filters_.size() > 0)
this doesn't seem to exactly match the logic at line 166 (that doesn't care about filters_.size().


http://gerrit.cloudera.org:8080/#/c/3873/18/be/src/exec/partitioned-hash-join-builder.h
File be/src/exec/partitioned-hash-join-builder.h:

Line 276:   /// Returns non-ok status if we couldn't spill a partition.
nit: weird line breaking.


PS18, Line 280: should have all of their build rows
what is this trying to say? that we're done partitioning the build?


PS18, Line 286: no buffers in either the build or probe stream.
clarify whether this means neither has buffers or at least one does not have buffers (the
latter being what the current wording seems to imply).


PS18, Line 287: in_mem
are these states actually represented in code? (if not, why the underscore?).
also, how about pointing at that for #2, no probe streams are needed.


PS18, Line 291: probe_buffers_for_spilled_partitions_
what's that?


PS18, Line 301: probe_buffers_for_spilled_partitions_
same


PS18, Line 349: PartitionedHashJoinNode
who owns it?


PS18, Line 427: so that the
              :   /// initial probe buffer is allocated
this doesn't really explain why the builder has to do it.  Would be good to explain that.
i.e. this can lead to more spilling


Line 432:   /// phase of the algorithm without spilling more partitions.
this hints at it, so maybe move this up and reword to be more explicit.


http://gerrit.cloudera.org:8080/#/c/3873/18/be/src/exec/partitioned-hash-join-node-ir.cc
File be/src/exec/partitioned-hash-join-node-ir.cc:

PS18, Line 285: *status
if we can now do this, can we also just make NextProbeRow() and others return Status directly?


Line 293:       if (LIKELY(hash_tbl != NULL)) {
is probe_partition NULL in this case?


http://gerrit.cloudera.org:8080/#/c/3873/18/be/src/exec/partitioned-hash-join-node.cc
File be/src/exec/partitioned-hash-join-node.cc:

Line 47:     "the memory limit may help this query to complete successfully.";
weird line breakages


http://gerrit.cloudera.org:8080/#/c/3873/18/be/src/exec/partitioned-hash-join-node.h
File be/src/exec/partitioned-hash-join-node.h:

Line 404:   /// builder_->hash_partitions. This is non-empty only in the PARTITIONING_PROBE
or
underscore, right?


PS18, Line 406: a
and?


-- 
To view, visit http://gerrit.cloudera.org:8080/3873
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I1e02ea9c7a7d1a0f373b11aa06c3237e1c7bd4cb
Gerrit-PatchSet: 18
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Michael Ho <kwho@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message