fix(plan): preserve ENUM and SET ordering through query boundaries - #26536
fix(plan): preserve ENUM and SET ordering through query boundaries#26536LeftHandCold wants to merge 6 commits into
Conversation
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
XuPeng-SH
left a comment
There was a problem hiding this comment.
Requesting changes for one concrete shared-path performance regression:
[P2] CastValueToIndex now builds the exact-label indexes once length*labels crosses the heuristic threshold. For legal case-insensitive but non-exact inputs, the exact lookup misses, and every row still falls back to the original linear ParseEnum scan after paying the O(labels) map-build cost. With a large constant ENUM definition and a short batch (length >= 8), this can be substantially slower and use much more peak memory than main. This affects existing batched INSERT/CAST workloads, not only the new planner-generated ORDER BY path.
Please either retain/use the EqualFold index on an exact miss (then parse a numeric ordinal only after name lookup misses), or isolate this optimization to the planner-generated exact-value path. Add exact/case-insensitive/numeric benchmarks across small and large definitions to justify the threshold.
Non-blocking but recommended in the same update: initialize mysqlSpecialOrderTypes and the per-binding provenance slice lazily, so ordinary non-ENUM query blocks and derived tables do not pay for this low-frequency feature.
The core planner-local provenance design and its correctness/unhappy-path coverage otherwise look sound. Scalar subqueries and non-reversible duplicate-label behavior are documented scope decisions, not blockers in this review.
XuPeng-SH
left a comment
There was a problem hiding this comment.
Additional blocking correctness finding: GROUP BY outputs fall out of the new provenance graph.
ProjectionBinder.BindExpr represents a grouped expression as ColRef{RelPos: groupTag} (projection_binder.go:44-53). However, mysqlSpecialOrderTypeForExpr only resolves a ColRef through projectTag or bindingByTag (mysql_special_types.go:276-285); groupTag is neither. The helper therefore returns nil, the ORDER BY rewrite is skipped, and the grouped ENUM/SET value is sorted as VARCHAR.
I reproduced this on the current PR head with a planner regression test. All of these produced a T_varchar sort/window key instead of T_enum/T_uint64:
select e from enum_order_t group by e order by e;
select e
from (select e from enum_order_t group by e) d
order by e;
with c as (select e from enum_order_t group by e)
select e from c order by e;
select s
from (select s from enum_order_t group by s) d
order by s;
select row_number() over (order by e)
from enum_order_t
group by e;As a control, the existing DISTINCT-derived path produced T_enum correctly. With labels low,mid,high, the grouped paths retain the original lexical high,low,mid failure, so this is not only missing metadata or test coverage.
Please carry the same narrow provenance through exact GROUP BY outputs (including the groupTag lookup/transfer), while continuing to clear it for expressions and incompatible definitions. Add ENUM and SET coverage for same-block, derived/CTE, and window ORDER BY after grouping; at least one execution-level BVT should assert the rows, not only the plan type.
XuPeng-SH
left a comment
There was a problem hiding this comment.
Requesting changes on 6736a8a56c48. The previous GROUP BY provenance gap, case-insensitive lookup semantics, and lazy allocation concerns are addressed, but two blocking issues remain.
-
[P1 correctness] Preserve ENUM/SET ordering in aggregate-local ORDER BY.
HavingBinder.bindGroupConcatOrderBystill callsgroupConcatOrderKey, which only unwraps a directcast_index_to_value/cast_index_to_set_valueexpression. After a derived-table or CTE boundary, the bound expression is a VARCHAR ColRef carrying the new planner provenance, so this path never consults it.I reproduced this deterministically on the current head by decoding the GROUP_CONCAT order-argument index from
AggConfig:select group_concat(e order by e) from enum_order_t-> order argumentT_enumselect group_concat(e order by e) from (select e from enum_order_t) d-> order argumentT_varchar- the equivalent SET-through-CTE case also remains string-typed
The executor therefore silently sorts the boundary cases lexically; for a definition such as
low,mid,high, that ishigh,low,midrather than definition order. This is the same provenance contract as top-level and window ORDER BY, not a separate scenario-specific feature.Please make aggregate-local order keys use the same provenance-aware conversion (while keeping explicit casts/expressions lexical and preserving the non-reversible-definition guard), and add ENUM and SET execution-level coverage across a query boundary.
-
[P2 performance] Do not enable the indexed shared cast where its own benchmark is slower.
The new threshold at
func_mo.go:1055-1073enables both maps for a 1,024-label definition with only 8 rows. Repeated runs of the PR's own benchmark (-benchtime=2000x -count=3) were stable:- exact: linear 69.2-69.8 us/op; indexed 77.5-78.1 us/op (~12% slower)
- numeric: linear 59.8-61.6 us/op; indexed 78.6-81.1 us/op (~30% slower)
- allocations: 8/op linear versus 1,062 exact / 1,070 numeric indexed
Intermediate threshold points (32, 64, 256, and 512 labels at 8 rows) were also at parity or slower while adding tens to hundreds of allocations. Eagerly constructing
foldedIndexesfor every label is paid even by exact and numeric inputs, sorows*labels >= 256is not a sufficient amortization model for this sharedCastValueToIndexpath.Please make the secondary index genuinely demand-driven or otherwise gate/isolate the optimization so every admitted workload is justified by the benchmark matrix, not only the 8-label and case-insensitive cases.
Validation completed on the exact head: both focused function tests passed under -race -count=100; both focused planner tests passed under -race -count=20; full pkg/sql/plan/function passed with race; both owning packages passed without race. Full pkg/sql/plan race was resource-inconclusive because the host cgroup OOM-killed it (the focused race suite passed).
XuPeng-SH
left a comment
There was a problem hiding this comment.
Requesting changes for one remaining correctness blocker on current head 87f13eca.
[P1] Treat SET definitions with an empty normalized member as non-reversible (pkg/sql/plan/mysql_special_types.go:337-339).
NormalizeSetValues proves that the member list is valid and case-insensitively unique, but it does not prove that the displayed SET value can be inverted to its original bitmap. For a legal definition such as SET('', 'a'), bitmap 0 and bitmap 1 both display as ""; ParseSet(",a", "") then returns 0 for both. The current helper returns true for this definition, and the derived/CTE ORDER BY path emits cast_set_value_to_index, silently collapsing two distinct sort keys.
A concrete execution-level reproduction is:
create table t(id int, s set('', 'a'));
insert into t values (2, 0), (1, 1);
select id, s from (select id, s from t) d order by s, id;Definition/bitmap order requires the bitmap-0 row (id=2) before the bitmap-1 row (id=1). On this head both reconstructed keys are 0, so the secondary key produces id=1, id=2 instead. I also reproduced the planner half directly: this query contains cast_set_value_to_index, while ParseSetIndex(",a", 0) == ParseSetIndex(",a", 1) and the inverse returns 0.
Please either carry the raw bitmap across this boundary (the newly merged SET bitmap materialization path from #26571 can already distinguish this case), or at minimum classify every normalized empty SET member as non-reversible and fail safely. Add a planner regression plus an execution BVT with the IDs arranged opposite bitmap order so a collapsed key cannot pass accidentally.
The previous GROUP BY, GROUP_CONCAT, EqualFold/indexing, and lazy-allocation findings are addressed. I reran the 64-row benchmark matrix on Apple M4: indexed exact/case-insensitive/numeric paths were consistently faster for every admitted 8-1024-label case. Focused race tests and both owning package suites also pass; current GitHub CI is green.
What type of PR is this?
Which issue(s) this PR fixes:
issue #26012
What this PR does / why we need it:
ENUM and SET values must keep MySQL definition-order sorting after a pure column passes through a relational query boundary. Current main loses the storage ordinal/bitmask at CTE and derived-table projections, then sorts the visible varchar value lexically.
This change carries planner-local ENUM/SET ordering provenance through pure passthrough projections, exact GROUP BY outputs, compatible set operations, recursive CTE members, aliases, top-level ORDER BY, window ORDER BY, and aggregate-local GROUP_CONCAT ORDER BY. The final sort key is reconstructed with the existing MySQL conversion functions. Literal NULL is neutral, while explicit casts, string expressions, literals, and incompatible definitions deliberately clear provenance and keep varchar semantics.
For duplicate or case-insensitively equivalent ENUM labels, the visible value cannot be reversed to a unique storage ordinal. Direct non-DISTINCT sorting continues to use the raw ordinal; boundary and direct DISTINCT cases fail safely instead of returning a silently wrong order.
The shared batch reverse conversion builds lazy exact and Unicode EqualFold indexes only for definitions with at least 8 labels and batches with at least 64 rows. Exact and case-insensitive labels use the index; numeric ordinals are parsed only after label lookup misses. Short definitions and smaller batches retain the existing parser path.
Ordinary non-ENUM query blocks and derived tables do not allocate provenance maps or per-binding provenance slices.
Scope exclusion: scalar subqueries are not changed because Expr_Sub is later flattened into a SINGLE JOIN and attaching conversion during expression binding could duplicate scalar-subquery execution.
Tested with deterministic MO CGo include/link/runtime paths:
go test -mod=readonly -count=1 ./pkg/sql/plango test -mod=readonly -count=1 ./pkg/sql/plan/functiongo test -mod=readonly -race -count=1 ./pkg/sql/plango test -mod=readonly -race -count=1 ./pkg/sql/plan/functionEach of these exact tests passed independently with
-race -count=100:TestMySQLSpecialOrderProvenanceThroughQueryBoundariesTestMySQLSpecialOrderProvenanceRejectsNonReversibleEnumTestMySQLSpecialOrderProvenanceInGroupConcatTestGroupConcatOrderKeyUsesEnumAndSetStorageValueTestCastValueToIndexConstDefinitionTestEnumValueIndexPreservesParseEnumSemanticsgo test -mod=readonly -run '^$' -bench '^BenchmarkEnumValueIndex$' -benchtime=2000x -benchmem -count=3 ./pkg/sql/plan/functiongo build -mod=readonly ./pkg/sql/plan ./pkg/sql/plan/functiongo vet -mod=readonly ./pkg/sql/plan ./pkg/sql/plan/functiongolangci-lint run ./pkg/sql/plan ./pkg/sql/plan/functionDistributed ENUM/SET BVT fixtures cover grouped and aggregate-local ordering through derived-table and CTE boundaries; the distributed runner was not started locally.