planner: align binary literal agg elimination with MySQL - #67343
planner: align binary literal agg elimination with MySQL#67343hawkingrei wants to merge 11 commits into
Conversation
|
Skipping CI for Draft Pull Request. |
ti-chi-bot
Bot
added
do-not-merge/needs-triage-completed
release-note-none
|
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 regression tests for inconsistent MIN on binary literals with HAVING when a table has a PRIMARY KEY, and updates aggregation-elimination cast wrapping to force casts for binary literals even when types appear equal. Changes
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 |
|
Skipping CI for Draft Pull Request. |
ti-chi-bot
Bot
added
size/M
|
/ok-to-test |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #67343 +/- ##
================================================
- Coverage 77.8039% 77.4344% -0.3696%
================================================
Files 2023 1943 -80
Lines 556185 549063 -7122
================================================
- Hits 432734 425164 -7570
- Misses 121706 123533 +1827
+ Partials 1745 366 -1379
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
ti-chi-bot
Bot
removed
do-not-merge/work-in-progress
![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/expression/aggregation/base_func.go (1)
334-339: Consider using the existingIsBinaryLiteralhelper for consistency.The codebase has an
expression.IsBinaryLiteralhelper function atpkg/expression/expression.go:1167-1171that performs the same check. Using it would reduce duplication and improve maintainability.The
Flencalculation (len(binaryLiteral) * 8) is correct—binary literal byte length multiplied by 8 gives the bit length, which aligns withmysql.TypeBitsemantics.♻️ Optional: Use existing helper
if a.Name == ast.AggFuncMax || a.Name == ast.AggFuncMin { - if con, ok := a.Args[0].(*expression.Constant); ok && con.Value.Kind() == types.KindBinaryLiteral { + if expression.IsBinaryLiteral(a.Args[0]) { + con := a.Args[0].(*expression.Constant) a.RetTp.SetType(mysql.TypeBit) a.RetTp.SetFlen(len(con.Value.GetBinaryLiteral()) * 8) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/expression/aggregation/base_func.go` around lines 334 - 339, Replace the in-place binary-literal check in the aggregation special-case with the existing helper: instead of testing "if con, ok := a.Args[0].(*expression.Constant); ok && con.Value.Kind() == types.KindBinaryLiteral" use the expression.IsBinaryLiteral helper (passing a.Args[0]) to detect binary literals and then set return type and Flen as before via a.RetTp.SetType(mysql.TypeBit) and a.RetTp.SetFlen(...). Update references around function/method names a.Name, ast.AggFuncMax/ast.AggFuncMin, expression.Constant, expression.IsBinaryLiteral and retain the Flen calculation (binary length * 8).
🤖 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/expression/aggregation/base_func.go`:
- Around line 334-339: Replace the in-place binary-literal check in the
aggregation special-case with the existing helper: instead of testing "if con,
ok := a.Args[0].(*expression.Constant); ok && con.Value.Kind() ==
types.KindBinaryLiteral" use the expression.IsBinaryLiteral helper (passing
a.Args[0]) to detect binary literals and then set return type and Flen as before
via a.RetTp.SetType(mysql.TypeBit) and a.RetTp.SetFlen(...). Update references
around function/method names a.Name, ast.AggFuncMax/ast.AggFuncMin,
expression.Constant, expression.IsBinaryLiteral and retain the Flen calculation
(binary length * 8).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 87b08d5c-360a-454e-8e11-7a79b5e90eb6
📒 Files selected for processing (3)
pkg/expression/aggregation/base_func.gopkg/expression/aggregation/base_func_test.gopkg/planner/core/integration_test.go
|
/retest |
ti-chi-bot
Bot
added
size/S
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/planner/core/integration_test.go (1)
2356-2356: Consider usingDROP TABLE IF EXISTSfor consistency.Line 2356 uses
drop table t70withoutIF EXISTS, while line 2346 usesdrop table if exists t70. UsingIF EXISTSconsistently makes the test more robust against potential failures in earlier statements.- tk.MustExec("drop table t70") + tk.MustExec("drop table if exists t70")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/planner/core/integration_test.go` at line 2356, Replace the unconditional drop with a safe conditional drop: locate the tk.MustExec call that executes "drop table t70" (the statement using tk.MustExec in integration_test.go) and change the SQL to use "DROP TABLE IF EXISTS t70" so the test remains robust and consistent with the earlier "drop table if exists t70" usage.
🤖 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/planner/core/integration_test.go`:
- Line 2356: Replace the unconditional drop with a safe conditional drop: locate
the tk.MustExec call that executes "drop table t70" (the statement using
tk.MustExec in integration_test.go) and change the SQL to use "DROP TABLE IF
EXISTS t70" so the test remains robust and consistent with the earlier "drop
table if exists t70" usage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ffa1cff2-9400-44c7-8494-b1551cc9a548
📒 Files selected for processing (2)
pkg/planner/core/integration_test.gopkg/planner/core/rule_aggregation_elimination.go
|
/retest |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/planner/core/integration_test.go`:
- Around line 2349-2354: The test is asserting empty results but should assert
the preserved BIT value; update the assertions for the queries using hex(c1) and
min(b'101010') (the two checks around the comment "issue:67039 with-pk" and its
non-PK counterpart) to expect "2A" instead of empty rows so the regression
verifies BIT semantics survive the HAVING filter (i.e., replace testkit.Rows()
with testkit.Rows("2A") for both occurrences).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 5709390e-ff9d-4c68-bc74-4fb05519973a
📒 Files selected for processing (1)
pkg/planner/core/integration_test.go
ti-chi-bot
Bot
added
size/M
ti-chi-bot
Bot
added
lgtm
and removed
needs-1-more-lgtm
| a.RetTp.DelFlag(mysql.NotNullFlag) | ||
| if (a.Name == ast.AggFuncMax || a.Name == ast.AggFuncMin) && expression.IsBinaryLiteral(a.Args[0]) { | ||
| con := a.Args[0].(*expression.Constant) | ||
| a.RetTp.SetType(mysql.TypeBit) |
There was a problem hiding this comment.
As shown in L332, a.RetTp is a clone of a.Args[0] type, i think the clone will also clone the flag information, so why need to add it here again?
There was a problem hiding this comment.
Good point. Clone() already preserves the original flags here, so there is no need to add them again.
In the current version of this patch I only override the logical type to TypeBit and set Flen from the binary literal width. The existing flag/charset/collation information still comes from the cloned argument type.
There was a problem hiding this comment.
I mean the type should not be overriden
There was a problem hiding this comment.
Thanks, I retried this direction in a detached worktree to check whether the planner-side CAST preservation alone would be enough.
It turns out it is not enough. The important point here is that this override is not re-applying cloned flags; it is intentionally correcting the logical return type for MIN/MAX over binary literals.
More concretely:
-
Clone()does preserve the existing flag / charset / collation metadata froma.Args[0]. I agree we do not need to add those again. -
But after the clone, the aggregate return type is still string-like. For a binary literal input, that is not sufficient for the aggregate result semantics we need here. This branch still needs to override the logical type to
mysql.TypeBitand setFlenfrom the binary literal width. -
I validated the reviewer-suggested direction directly: keep the planner-side CAST guard, but remove only the
TypeBitoverride inbase_func.go. That immediately regresses two places:./tools/check/failpoint-go-test.sh pkg/expression/aggregation -run '^TestBaseFunc_InferAggRetType$' -count=1- the inferred type falls back to
TypeVarString
- the inferred type falls back to
./tools/check/failpoint-go-test.sh pkg/planner/core -run '^TestIssue66619$' -count=1issue:67039with PK returns empty again
-
So the problem is earlier than the planner rule. Once aggregate type inference falls back to
TypeVarString, the binary-literal semantics are already wrong before the planner-side CAST guard can help. The planner guard only preserves one downstream planner path; it cannot repair an already-wrong aggregate return type. -
That is why the current split is still needed:
- clone the original type to preserve the existing metadata
- override the logical type/width to
TypeBitfor the binary-literalMIN/MAXresult itself
So I do not think we can remove the type override as a narrow change in this PR. If we want to eliminate this override entirely, that likely needs a broader binary-literal typing cleanup instead of only relying on the planner-side CAST preservation.
There was a problem hiding this comment.
I tried removing only this TypeBit override while keeping the planner-side CAST preservation. That regresses both ./tools/check/failpoint-go-test.sh pkg/expression/aggregation -run ^TestBaseFunc_InferAggRetType$ -count=1 (MAX/MIN(binary literal) falls back to TypeVarString) and ./tools/check/failpoint-go-test.sh pkg/planner/core -run ^TestIssue66619$ -count=1 (the issue:67039 with-pk case becomes empty again instead of returning 2A). Since binary literals are globally typed as TypeVarString today, the planner-side CAST guard alone cannot recover BIT semantics; avoiding this override would need a broader binary-literal typing redesign rather than a local fix in this PR.
There was a problem hiding this comment.
In my understanding, b'101010' is a literal of type bit, so in TiDB the expected behavior will be that the input of MIN is type bit, and the output of MIN should also be type bit, it is not related to string like type, b'101010' is a Bit literal, i don't understand why in TiDB it will be treated as string like type.
There was a problem hiding this comment.
You're right to question the TypeBit override here. I rechecked the behavior against MySQL 5.7, and that override is not the right fix for this PR.
In TiDB today, binary literals still flow through this path with a string-like field type. Changing that globally would be a broader typing cleanup, but it is not needed for issue #67039. The actual MySQL-compatible fix here is planner-side only: keep the CAST during aggregation elimination so the eliminated path does not expose the raw literal in HAVING.
With that change, both query shapes now match MySQL:
select hex(c1) ...returns2Aselect hex(c1) ... having c1returnsEmpty set
I have removed the base_func.go override in the latest update and rewrote the regression accordingly.
hawkingrei
changed the title
ti-chi-bot
Bot
added
do-not-merge/needs-tests-checked
and removed
do-not-merge/needs-tests-checked
labels
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: qw4990, winoros The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/hold |
ti-chi-bot
Bot
added
the
do-not-merge/hold
|
@hawkingrei: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
Closing this PR because current |
4 participants
What problem does this PR solve?
Issue Number: close #67039
Problem Summary:
For
MIN(b'101010')in a derived query withHAVING c1, TiDB could return different results depending on whether aggregation elimination happened. The eliminated branch could expose the raw binary literal inHAVING, which diverged from MySQL.What changed and how does it work?
CASTfor binary literals in aggregation elimination even when the argument type already compares equal to the target type.MAX/MIN(binary literal)TypeBitoverride from this PR.TestIssue66619for both with-PK and without-PK query shapes.select hex(c1) ...returns2Aselect hex(c1) ... having c1returnsEmpty setCheck List
Tests
Manual test:
create table t70(c0 char, primary key(c0)); insert into t70(c0) values (1);select hex(c1) from (select 1 as c0, min(b'101010') as c1 from t70 group by t70.c0, t70.c0) s;2Aselect hex(c1) from (select 1 as c0, min(b'101010') as c1 from t70 group by t70.c0, t70.c0 having c1) s;Empty setHAVINGresult is alsoEmpty setwithout the primary keySide effects
Documentation
Release note