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 33: (14 comments) Sorry for missing your comments on PS33. http://gerrit.cloudera.org:8080/#/c/14859/33/be/src/exec/join-builder.h File be/src/exec/join-builder.h: http://gerrit.cloudera.org:8080/#/c/14859/33/be/src/exec/join-builder.h@137 PS33, Line 137: stuck > nit: extra word? Done http://gerrit.cloudera.org:8080/#/c/14859/33/be/src/exec/join-builder.cc File be/src/exec/join-builder.cc: http://gerrit.cloudera.org:8080/#/c/14859/33/be/src/exec/join-builder.cc@39 PS33, Line 39: JoinBuilder::~JoinBuilder() {} > maybe DCHECK probe_refcount_ and outstanding_probes_ == 0 Did this for probe_refcount_. It isn't an invariant for outstanding_probes_ since the query can get cancelled before the builder calls Handoff...() or before all the probes call Wait..(). http://gerrit.cloudera.org:8080/#/c/14859/33/be/src/exec/join-builder.cc@68 PS33, Line 68: or > nit: extra word Done http://gerrit.cloudera.org:8080/#/c/14859/33/be/src/exec/join-builder.cc@94 PS33, Line 94: { > nit: dont need enclosing brackets Done http://gerrit.cloudera.org:8080/#/c/14859/35/be/src/exec/partitioned-hash-join-builder.h File be/src/exec/partitioned-hash-join-builder.h: http://gerrit.cloudera.org:8080/#/c/14859/35/be/src/exec/partitioned-hash-join-builder.h@a504 PS35, Line 504: > did you miss adding this back? (here and below) This was deliberately removed since they would've forced it onto a new line - they're not all that useful now that the precommit builds with clang automatically catch dropped statuses. http://gerrit.cloudera.org:8080/#/c/14859/33/be/src/exec/partitioned-hash-join-builder.h File be/src/exec/partitioned-hash-join-builder.h: http://gerrit.cloudera.org:8080/#/c/14859/33/be/src/exec/partitioned-hash-join-builder.h@456 PS33, Line 456: For NAAJ, we need 3 additional buffers for 'null_aware_partition_', : /// 'null_aware_probe_partition_' and 'null_probe_rows_'. > nit: update comment Done http://gerrit.cloudera.org:8080/#/c/14859/33/be/src/exec/partitioned-hash-join-builder.h@636 PS33, Line 636: was > nit: typo Done http://gerrit.cloudera.org:8080/#/c/14859/33/be/src/exec/partitioned-hash-join-builder.h@636 PS33, Line 636: was > nit: typo Done http://gerrit.cloudera.org:8080/#/c/14859/33/be/src/exec/partitioned-hash-join-node.cc File be/src/exec/partitioned-hash-join-node.cc: http://gerrit.cloudera.org:8080/#/c/14859/33/be/src/exec/partitioned-hash-join-node.cc@78 PS33, Line 78: RETURN_IF_ERROR(PhjBuilderConfig::CreateConfig(state, tnode_->node_id, : tnode_->hash_join_node.join_op, &build_row_desc(), eq_join_conjuncts, : tnode_->runtime_filters, tnode_->hash_join_node.hash_seed, &phj_builder_config)); > this can be optional now based on UseSeparateBuild Done. Had to move IsSeparateBuild() into the plan node. http://gerrit.cloudera.org:8080/#/c/14859/33/be/src/exec/partitioned-hash-join-node.cc@723 PS33, Line 723: output_unmatched_batch_->TransferResourceOwnership(out_batch); : output_unmatched_batch_->Reset(); > why is this done before and after? AH, this was a merge issue with IMPALA-9349, I can remove this code change since it's fixed in IMPALA-9349. http://gerrit.cloudera.org:8080/#/c/14859/33/be/src/runtime/coordinator.cc File be/src/runtime/coordinator.cc: http://gerrit.cloudera.org:8080/#/c/14859/33/be/src/runtime/coordinator.cc@363 PS33, Line 363: src_node_id, > this is kind of confusing because in this case the sink produces the filter I added an explanatory comment. It is a little weird, but it didn't seem worth trying to rework how the filter code identifies sources. http://gerrit.cloudera.org:8080/#/c/14859/33/be/src/runtime/runtime-state.cc File be/src/runtime/runtime-state.cc: http://gerrit.cloudera.org:8080/#/c/14859/33/be/src/runtime/runtime-state.cc@249 PS33, Line 249: > nit: extra line Done http://gerrit.cloudera.org:8080/#/c/14859/33/common/thrift/PlanNodes.thrift File common/thrift/PlanNodes.thrift: http://gerrit.cloudera.org:8080/#/c/14859/33/common/thrift/PlanNodes.thrift@367 PS33, Line 367: optional > should this be required instead? We're pretty inconsistent about this. I think it mostly doesn't matter. Required fields are generally considered a bad practice in thrift because of compatibility, but that isn't a real consideration here. The main downside of required fields that I've seen is that the APIs are a bit clunkier, and it's harder to do thinks like assert that a value was actually set. http://gerrit.cloudera.org:8080/#/c/14859/33/common/thrift/PlanNodes.thrift@617 PS33, Line 617: 28 > nit: would it might make more sense to have hash_join_node or nested_loop_j That's a very good point. -- 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: 33 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: Wed, 12 Feb 2020 04:52:18 +0000 Gerrit-HasComments: Yes
