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 |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a conservative runtime-filter compatibility guard in the join planner that disables RF generation when join key protobuf field types, integer signed/unsigned flags, or decimal precision/scale differ. Adds GoogleTest cases verifying RF is not used/pruned when such mismatches occur. 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.
12f0add to
8dfcf74
Compare
|
/retest |
1 similar comment
|
/retest |
8dfcf74 to
1ae7ec8
Compare
1ae7ec8 to
3f40c13
Compare
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