Copilot commented on code in PR #63482:
URL: https://github.com/apache/doris/pull/63482#discussion_r3280159649
##########
be/src/exec/operator/partitioned_hash_join_probe_operator.cpp:
##########
@@ -542,6 +542,9 @@ Status PartitionedHashJoinProbeOperatorX::init(const
TPlanNode& tnode, RuntimeSt
// default repartition max depth; can be overridden from session variable
_repartition_max_depth = state->spill_repartition_max_depth();
RETURN_IF_ERROR(JoinProbeOperatorX::init(tnode, state));
+ if (_is_mark_join && _join_op == TJoinOp::RIGHT_ANTI_JOIN) {
+ return Status::InternalError("Hash join does not support right anti
mark join");
Review Comment:
This new InternalError message doesn't include query/node context, which can
make diagnosing malformed plans harder in production logs. Consider adding
query id + node id (and/or to_string(_join_op) / debug_string) to the error so
operators can be located quickly when this triggers.
##########
be/src/exec/operator/hashjoin_probe_operator.cpp:
##########
@@ -461,6 +461,9 @@ Status HashJoinProbeOperatorX::push(RuntimeState* state,
Block* input_block, boo
Status HashJoinProbeOperatorX::init(const TPlanNode& tnode, RuntimeState*
state) {
RETURN_IF_ERROR(JoinProbeOperatorX<HashJoinProbeLocalState>::init(tnode,
state));
DCHECK(tnode.__isset.hash_join_node);
+ if (_is_mark_join && _join_op == TJoinOp::RIGHT_ANTI_JOIN) {
+ return Status::InternalError("Hash join does not support right anti
mark join");
+ }
Review Comment:
The same right-anti mark-join validation is duplicated across multiple hash
join operator init() implementations (build/probe, partitioned/unpartitioned).
This duplication risks the checks diverging over time; consider factoring it
into a shared helper (e.g., in JoinBuildSinkOperatorX/JoinProbeOperatorX base
init) so all hash-join variants enforce the same supported mark-join shapes in
one place.
##########
be/src/exec/operator/hashjoin_build_sink.cpp:
##########
@@ -700,6 +700,9 @@
HashJoinBuildSinkOperatorX::HashJoinBuildSinkOperatorX(ObjectPool* pool, int ope
Status HashJoinBuildSinkOperatorX::init(const TPlanNode& tnode, RuntimeState*
state) {
RETURN_IF_ERROR(JoinBuildSinkOperatorX::init(tnode, state));
DCHECK(tnode.__isset.hash_join_node);
+ if (_is_mark_join && _join_op == TJoinOp::RIGHT_ANTI_JOIN) {
+ return Status::InternalError("Hash join does not support right anti
mark join");
+ }
Review Comment:
New behavior is introduced here (rejecting right anti mark join) but the
added unit test only asserts HashJoinProbeOperatorX::init() fails; the same
check was also added to build-sink and partitioned hash-join operators. Please
add/extend unit tests (e.g., in hashjoin_build_sink_test.cpp and
partitioned_hash_join_*_operator_test.cpp) to cover these additional init()
rejection paths so regressions are caught.
##########
be/test/exec/operator/hashjoin_probe_operator_test.cpp:
##########
@@ -919,6 +919,17 @@ TEST_F(HashJoinProbeOperatorTest, RightSemiJoinMarkJoin) {
Field::create_field<TYPE_BOOLEAN>(1),
Field::create_field<TYPE_BOOLEAN>(0)});
}
+TEST_F(HashJoinProbeOperatorTest, RightAntiMarkJoinRejected) {
+ auto tnode = _helper.create_test_plan_node(TJoinOp::RIGHT_ANTI_JOIN,
{TPrimitiveType::INT},
+ {false}, {false}, true, 1);
Review Comment:
This test constructs a mark-join plan with
key_types.size()==mark_join_conjuncts_size, which (via
HashJoinTestHelper::create_test_plan_node) results in eq_join_conjuncts being
empty. To make the test less fragile and closer to real plans, consider
ensuring at least one eq_join_conjunct remains (e.g., by using 2 key columns
and mark_join_conjuncts_size=1) so the failure is clearly attributable to the
right-anti mark-join rejection.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
