Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/14859 )
Change subject: IMPALA-4224: execute separate join builds fragments ...................................................................... Patch Set 34: (11 comments) http://gerrit.cloudera.org:8080/#/c/14859/28/be/src/exec/blocking-join-node.cc File be/src/exec/blocking-join-node.cc: http://gerrit.cloudera.org:8080/#/c/14859/28/be/src/exec/blocking-join-node.cc@231 PS28, Line 231: _sink) { > is it still true? The phrase was a bit weird, this was mean to be part of the "if" clause. I rewrote this comment to be clearer and explain the bigger picture. http://gerrit.cloudera.org:8080/#/c/14859/28/be/src/exec/blocking-join-node.cc@246 PS28, Line 246: seSeparateBuild(state->query_options())) { > Maybe you could add a sentence about how the build was already started. Done http://gerrit.cloudera.org:8080/#/c/14859/32/be/src/exec/nested-loop-join-builder.h File be/src/exec/nested-loop-join-builder.h: http://gerrit.cloudera.org:8080/#/c/14859/32/be/src/exec/nested-loop-join-builder.h@61 PS32, Line 61: NljBuilder( > nit: maybe it could be another factory method called CreateStandaloneBuilde Done http://gerrit.cloudera.org:8080/#/c/14859/32/be/src/exec/nested-loop-join-builder.h@90 PS32, Line 90: util > nit: until Done http://gerrit.cloudera.org:8080/#/c/14859/32/be/src/exec/nested-loop-join-builder.cc File be/src/exec/nested-loop-join-builder.cc: http://gerrit.cloudera.org:8080/#/c/14859/32/be/src/exec/nested-loop-join-builder.cc@112 PS32, Line 112: void NljBuilder::Reset() { > Based on the comment on the declaration it is not valid to be called on sep Done http://gerrit.cloudera.org:8080/#/c/14859/32/be/src/exec/partitioned-hash-join-builder.h File be/src/exec/partitioned-hash-join-builder.h: http://gerrit.cloudera.org:8080/#/c/14859/32/be/src/exec/partitioned-hash-join-builder.h@103 PS32, Line 103: TDataSink* tsink > Since now it is an output parameter should it be moved at the end of the pa Done http://gerrit.cloudera.org:8080/#/c/14859/32/be/src/exec/partitioned-hash-join-builder.h@322 PS32, Line 322: 'a > nit: missing space Done http://gerrit.cloudera.org:8080/#/c/14859/32/be/src/exec/partitioned-hash-join-builder.cc File be/src/exec/partitioned-hash-join-builder.cc: http://gerrit.cloudera.org:8080/#/c/14859/32/be/src/exec/partitioned-hash-join-builder.cc@370 PS32, Line 370: void PhjBuilder::Reset(RowBatch* row_batch) { > The declaration comment says it's not valid to be called on a separate buil Done http://gerrit.cloudera.org:8080/#/c/14859/32/be/src/runtime/bufferpool/buffer-pool.cc File be/src/runtime/bufferpool/buffer-pool.cc: http://gerrit.cloudera.org:8080/#/c/14859/32/be/src/runtime/bufferpool/buffer-pool.cc@28 PS32, Line 28: #include "util/debug-util.h" > nit: not in alphabetic order, should be one line below Done http://gerrit.cloudera.org:8080/#/c/14859/32/be/src/runtime/coordinator.cc File be/src/runtime/coordinator.cc: http://gerrit.cloudera.org:8080/#/c/14859/32/be/src/runtime/coordinator.cc@322 PS32, Line 322: fragment.output_sink.type == TDataSinkType::DATA_STREAM_SINK : || fragment.output_sink.type == TDataSinkType::HASH_JOIN_BUILDER : || fragment.output_sink.type == TDataSinkType::NESTED_LOOP_JOIN_BUILDER > nit: I wonder if these conditions could be simplified if some parts of them Factored out an IsJoinBuildSink() function. I thought about something to capture this full condition like IsFragmentConnectingSink() but it didn't seem like it made things clearer. http://gerrit.cloudera.org:8080/#/c/14859/32/common/thrift/PlanNodes.thrift File common/thrift/PlanNodes.thrift: http://gerrit.cloudera.org:8080/#/c/14859/32/common/thrift/PlanNodes.thrift@617 PS32, Line 617: 28 > nit: Since these thrift structures only used between Impala daemons with th I did this mainly to reduce the size of the diff. I think I'd prefer to keep it this way but I can change if you feel strongly - neither option is that good in my mind. -- To view, visit http://gerrit.cloudera.org:8080/14859 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4403c8e62d9c13854e7830602ee613f8efc80c58 Gerrit-Change-Number: 14859 Gerrit-PatchSet: 34 Gerrit-Owner: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: Bikramjeet Vig <bikramjeet....@cloudera.com> Gerrit-Reviewer: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: Zoltan Borok-Nagy <borokna...@cloudera.com> Gerrit-Comment-Date: Mon, 03 Feb 2020 17:05:56 +0000 Gerrit-HasComments: Yes
