executor: disable runtime filter on join key type mismatch#10698
executor: disable runtime filter on join key type mismatch#10698ChangRui-Ryan wants to merge 1 commit intopingcap:masterfrom
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
📝 WalkthroughWalkthroughAdds a conservative compatibility check that disables generation of join runtime filters when join key protobuf field types or integer signed/unsigned flags differ; Changes
Sequence Diagram(s)(Skipped) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
91d4f88 to
1f12fc2
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@dbms/src/Flash/Planner/Plans/PhysicalJoin.cpp`:
- Around line 173-212: Rename the snake_case local symbols to camelCase: change
the lambda is_join_key_field_type_compatible to isJoinKeyFieldTypeCompatible,
the bool enable_runtime_filter to enableRuntimeFilter, and runtime_filter_list
to runtimeFilterList; update the lambda invocation and its use sites (the const
bool assignment and the ternary that builds runtimeFilterList using
tiflash_join.genRuntimeFilterList) so all references match the new camelCase
names and keep behavior unchanged.
1f12fc2 to
f61f401
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@dbms/src/Flash/tests/gtest_runtime_filter_disable_on_type_mismatch.cpp`:
- Around line 43-49: The loop variable name in the WrapForRuntimeFilterTestBegin
macro is declared as enablePipeline but the test code uses enable_pipeline,
causing a compile error; fix by making the identifier consistent—either change
the macro's loop variable to enable_pipeline or update all references (the
usages currently named enable_pipeline) to enablePipeline so the symbol matches,
ensuring WrapForRuntimeFilterTestBegin and all occurrences use the same
identifier.
🧹 Nitpick comments (2)
dbms/src/Flash/Planner/Plans/PhysicalJoin.cpp (1)
161-199: Conservative guard logic looks correct; minor style nit on redundantcontinue.The lambda correctly checks:
- Equal number of join keys
- Both keys have valid field types
- Same protobuf type (
tp())- Same unsigned flag
One minor observation: the
continue;on line 196 is redundant as it's at the end of the for-loop body.🧹 Optional: remove redundant continue
// Otherwise keep conservative: if we got here (same tp + same unsigned flag), treat as compatible. - continue; } return true; };dbms/src/Flash/tests/gtest_runtime_filter_disable_on_type_mismatch.cpp (1)
43-49: Use the existingWRAP_FOR_TEST_BEGINandWRAP_FOR_TEST_ENDmacros fromExecutorTestUtils.h.The file already imports
ExecutorTestUtils.hand extendsExecutorTest, making it an executor test that should follow the established pattern. The customWrapForRuntimeFilterTestBegin/Endmacros duplicate the existingWRAP_FOR_TEST_BEGIN/ENDmacros and are already used consistently throughout other executor tests indbms/src/Flash/tests/. Replace lines 43-49 with the standard macros to maintain consistency with the codebase pattern.
a36018a to
12f0add
Compare
12f0add to
8dfcf74
Compare
|
/retest |
What problem does this PR solve?
Issue Number: close #10699
Problem Summary:
What is changed and how it works?
Check List
Tests
Side effects
Documentation
Release note
Summary by CodeRabbit
Bug Fixes
Tests