expression: avoid mutating shared columns in case results - #68079
expression: avoid mutating shared columns in case results#68079hawkingrei wants to merge 4 commits into
Conversation
ti-chi-bot
Bot
added
release-note
📝 WalkthroughWalkthroughNormalize CASE result arms' charset/collation when the inferred CASE result type is string, add a regression test for Issue Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
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 |
|
/retest |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/executor/test/issuetest/executor_issue_test.go (1)
803-814: Pin the non-binary collation in the test DDL.This regression hinges on
t1.c0 <= t0.c0staying true under the column collation and flipping only when the CASE arm is coerced to binary, but both tables currently inherit whatever default collation the test environment uses. Makingc0explicit would keep the reproduction deterministic and easier to understand.As per coding guidelines, "Keep test changes minimal and deterministic; avoid broad golden/testdata churn unless required."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/executor/test/issuetest/executor_issue_test.go` around lines 803 - 814, The test relies on t1.c0 <= t0.c0 using a non-binary collation but the DDL currently inherits the environment default; modify the CREATE TABLE for issue_67375_t0 and issue_67375_t1 to pin c0 to an explicit non-binary collation (e.g. add "c0 text COLLATE <non-binary-collation>") so the comparisons in the test (references to t1.c0, t0.c0 and the CASE expressions used in issueFilter) are deterministic regardless of test environment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/executor/test/issuetest/executor_issue_test.go`:
- Around line 803-814: The test relies on t1.c0 <= t0.c0 using a non-binary
collation but the DDL currently inherits the environment default; modify the
CREATE TABLE for issue_67375_t0 and issue_67375_t1 to pin c0 to an explicit
non-binary collation (e.g. add "c0 text COLLATE <non-binary-collation>") so the
comparisons in the test (references to t1.c0, t0.c0 and the CASE expressions
used in issueFilter) are deterministic regardless of test environment.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: fdbc8597-14f3-48d3-ac3c-3ffb832f7098
📒 Files selected for processing (4)
pkg/executor/test/issuetest/BUILD.bazelpkg/executor/test/issuetest/executor_issue_test.gopkg/expression/builtin_control.gopkg/expression/builtin_control_test.go
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #68079 +/- ##
================================================
- Coverage 77.7777% 77.0820% -0.6957%
================================================
Files 1990 1972 -18
Lines 551502 552965 +1463
================================================
- Hits 428946 426237 -2709
- Misses 121636 126653 +5017
+ Partials 920 75 -845
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
/retest |
hawkingrei
added
AI-Correction
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: qw4990 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 |
[LGTM Timeline notifier]Timeline:
|
|
/retest |
| // signature is built. See wrapCaseWhenStringResult for why the normal | ||
| // string argument wrapping is not enough here. | ||
| if tp == types.ETString { | ||
| args[i+1] = wrapCaseWhenStringResult(ctx, args[i+1], fieldTp) |
There was a problem hiding this comment.
This fixes the CASE path, but IF() and IFNULL() in the same file still pass raw string result arms into newBaseBuiltinFuncWithFieldTypes, whose ETString branch only does WrapWithCastAsString and can therefore keep reusing an existing string Column node. If the leak here comes from that shared-node coercion, the same pattern can still be triggered with IF(cond, text_col, blob_col) or IFNULL(text_col, blob_col). It would be safer to move this protection into the shared string-coercion path or mirror it in the other control-function builders.
3 participants
What problem does this PR solve?
Issue Number: close #67375
Related Issue: ref #64822
Problem Summary:
A
CASEexpression with a binary string result could reuse a non-binary string column expression as one of its result arms. The binaryCASEcontext could mutate shared column expression metadata, so the same column used by theJOINcomparison was compared with the wrong collation and made a logically contradictory aggregate return a non-zero count.What changed and how does it work?
This PR wraps
CASEstring result arms as expression copies with the resolvedCASEcharset and collation before building the builtin signature. The cast is applied only toCASEresult arm copies, so shared column expressions keep their original charset and collation when they are also used elsewhere, such as inJOINpredicates.The change intentionally avoids the global
ETStringimplicit-cast path.Check List
Tests
Side effects
Documentation
Release note
Tests
./tools/check/failpoint-go-test.sh pkg/expression -run TestCaseWhen -count=1./tools/check/failpoint-go-test.sh pkg/executor/test/issuetest -run TestCaseWhenBinaryResultDoesNotMutateSharedColumns -count=1make bazel_preparemake lintgit diff --check -- pkg/expression/builtin_control.go pkg/expression/builtin_control_test.go pkg/executor/test/issuetest/executor_issue_test.go pkg/executor/test/issuetest/BUILD.bazelSummary by CodeRabbit
Bug Fixes
Tests
Chores