impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Tim Armstrong (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-3567: Part 1: groundwork to make Join build sides DataSinks
Date Mon, 08 Aug 2016 22:30:21 GMT
Tim Armstrong has posted comments on this change.

Change subject: IMPALA-3567: Part 1: groundwork to make Join build sides DataSinks
......................................................................


Patch Set 2:

(15 comments)

http://gerrit.cloudera.org:8080/#/c/3842/1/be/src/exec/blocking-join-node.cc
File be/src/exec/blocking-join-node.cc:

Line 195:           &build_side_status));
> indent 2 more
Done.


http://gerrit.cloudera.org:8080/#/c/3842/1/be/src/exec/blocking-join-node.h
File be/src/exec/blocking-join-node.h:

Line 67:   boost::scoped_ptr<RowBatch> build_batch_;
> is this only utilized when the join runs in a particular 'mode' (i don't me
The two changes are unrelated. The build_pool_ was only used by the legacy HashJoinNode, so
I moved it there. The build_batch_ is added as an optimisation for Reset() to avoid reallocating
the batch.


Line 122:   /// Set up 'current_probe_row_' to point to the first input row from the left
child
> you mean from probe_batch_? i don't see anything in this function that rest
Right, my goal here was to change the control flow of the join nodes only to the extent that
I needed to add the join sink. In practice the join nodes just need to call this at the end
of Open() to prime themselves for processing the probe input.

It's hard to abstract over the global state in the join node without this turning into even
more of a rewrite of the three join nodes so it seemed better to document the postconditions.
E.g. I remember Michael looked at consolidating probe_batch_pos_ and current_probe_row_ and
even that was somewhat non-trivial.

It does make some assumptions about the state of the node that aren't going to be true if
it's partway through probing. E.g. it assumes that probe_batch_ is empty and that probe_side_eos_
is false. So I wanted to name it in a way that made it clear that you should only call it
at the start. I added DCHECKs for a couple of those assumptions.


Line 202:   void ProcessBuildInputAsync(RuntimeState* state, DataSink* build_sink,
> find a verb for this? ConstructBuildSide?
ProcessBuildInputAsync()


http://gerrit.cloudera.org:8080/#/c/3842/1/be/src/exec/data-sink.h
File be/src/exec/data-sink.h:

Line 108: 
> simply 'not owned' is fine (unless there's some special dependency that thi
Done


http://gerrit.cloudera.org:8080/#/c/3842/1/be/src/exec/hdfs-table-sink.cc
File be/src/exec/hdfs-table-sink.cc:

Line 631:     PartitionPair* dummy;
> call this dummy or something to that effect, to signal that it's not needed
Done


http://gerrit.cloudera.org:8080/#/c/3842/1/be/src/exec/hdfs-table-sink.h
File be/src/exec/hdfs-table-sink.h:

Line 194:       PartitionPair** partition_pair, bool no_more_rows);
> odd indentation
Done


http://gerrit.cloudera.org:8080/#/c/3842/1/be/src/exec/nested-loop-join-builder.cc
File be/src/exec/nested-loop-join-builder.cc:

Line 31:     MemTracker* mem_tracker) :
> move initializer to new line
Done


http://gerrit.cloudera.org:8080/#/c/3842/1/be/src/exec/nested-loop-join-builder.h
File be/src/exec/nested-loop-join-builder.h:

Line 35: /// tuple memory, the non-copying mode is used and row batches are simply accumulated
in
> meaning that you have to call AddBuildBatch and can't call Send?
It switches depending on whether the row batches have the 'need_to_return' flag set. I tried
to see if there were any reasonable DCHECKs I could add to enforce this further but there
wasn't anything straightforward.


Line 90:   /// memory that is owned by the child node.
> the meaning of 'raw' wasn't clear to me until after i read the .cc (we typi
Extended comment to clarify.


Line 92: 
> "deep copied out of raw_build_batches_"
Done


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

Line 247:   for (const FilterContext& filter: filters_) RETURN_IF_ERROR(filter.expr->Open(state));
> single line if it fits
Done


http://gerrit.cloudera.org:8080/#/c/3842/1/be/src/exec/row-batch-cache.h
File be/src/exec/row-batch-cache.h:

Line 70:   /// GetNextBatch() has not yet been called.
> has not yet been called
Done


http://gerrit.cloudera.org:8080/#/c/3842/1/be/src/runtime/plan-fragment-executor.cc
File be/src/runtime/plan-fragment-executor.cc:

Line 65: PlanFragmentExecutor::PlanFragmentExecutor(ExecEnv* exec_env,
> that seems generally useful. move to RowBatch?
Done


http://gerrit.cloudera.org:8080/#/c/3842/1/be/src/runtime/row-batch.cc
File be/src/runtime/row-batch.cc:

Line 37: 
> ?
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I9d7608181eeacfe706a09c1e153d0a3e1ee9b475
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <marcel@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message