Repository navigation
fix(sql): preserve signed zero in float set operations - #30184
iamlinjunhong wants to merge 40 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? |
73912ed to
d48be22
Compare
|
mo-bot review · Commit: Review completed. Read the assessment. |
XuPeng-SH
left a comment
There was a problem hiding this comment.
mo-bot review · mo-bot-local · review
Commit: d48be22e128ef9f3de3e0ff924533ab9626d867e
Assessment: request_changes
Reviewed the complete fourteen-file delta and affected consumers. Reusing physical equality keys is a sound way to isolate signed-zero set semantics. However, integration with correlated-query handling and predicate pushdown introduces two confirmed regressions.
Model: gpt-6-astra / xhigh
Session: 01a12d8f-7dea-77c2-a565-38d45b5564e4 · slot 0 · generation 0
-
P2
pkg/sql/plan/query_builder.go:4868: On t(a INT,b INT,f DOUBLE), execute SELECT a FROM t o WHERE EXISTS (SELECT f FROM t i WHERE i.a=o.a AND i.b=o.b UNION SELECT f FROM t i WHERE i.a=o.a AND i.b=o.b). Identical arms using i.a>o.a also trigger the regression.
Impact: Previously supported queries now fail with error 20102. Prepared versions also fail when both arms select the integer constant 1. Preserve these correlation shapes when retaining float or prepared arms.
Evidence: All six public SQL probes pass with exact merge-base overlays. The candidate fails float/prepared multiple-predicate and range-predicate cases; single-equality controls pass. The new collapse restrictions expose flatten_subquery.go:4388–4404, which accepts only one matching equality identity per arm. Results are retained in public-base.log and public-head.log. -
P2
pkg/sql/plan/query_builder.go:4861: On t(k INT PRIMARY KEY,d DOUBLE), execute SELECT k,d FROM (SELECT k,d FROM t UNION SELECT k,d FROM t) u WHERE k=1.
Impact: Adding float physical keys activates the blanket filter-pushdown barrier, converting a selective primary-key lookup into full scans and hash aggregation. Preserve pushdown for predicates proven invariant under the equality key, including this predicate on the untouched integer column.
Evidence: With 4,096 finite rows, EXPLAIN PHYPLAN ANALYZE changes from one scanned row and 37 bytes of reported memory on base to two 4,096-row scans, an 8,192-input-row aggregate, and 761,904 bytes on HEAD. The integer-only control remains one scanned row and 25 bytes. The filter stays above aggregation because pushdown.go:944 rejects pushdown whenever PhysicalEqualityKeyList is populated. See perf-base.log and perf-head.log.
Validation:
-
Artifacts: /home/xupeng/.local/state/mo-bot/jobs/cf9b4aade4ab97927e6c6434/attempt-1. evidence-audit.json reconciles seven execution receipts with commands, source fingerprints, logs and overlay inputs. Final audit receipt 1792012368798373316-4.json exits 0; source remains unchanged.
-
Isolation preflight passed. Native provenance verified 192 current source inputs and 62 artifacts against the retained repository build-owner receipt and log.
-
Focused function, planner, group/spill and compile/remote-codec tests passed: receipt 1792012253239799077-3.json.
-
TestSetOperationSignedZero passed. Independent correlation probes passed on base and exposed four candidate failures: receipts 1792012264966570110-3.json and 1792012269810977520-3.json.
-
Both physical-work comparisons completed successfully: receipts 1792012294035663064-3.json and 1792012294734351744-3.json.
-
Frozen exact-head CI reports successful UT, static analysis, shared build, proxy/pessimistic multi-CN BVT and coverage.
Limitations:
-
The full linked issue body, raw CI logs and retained author validation artifacts were not supplied; local_validation was empty.
-
Local public SQL probes used one CN. Codec and compilation checks do not constitute a local multi-CN runtime run.
-
Performance evidence measures execution-plan work and memory, not sustained latency or concurrency. Full repository, BVT and race suites were not rerun locally.
What type of PR is this?
Which issue(s) this PR fixes:
What this PR does / why we need it:
MySQL 8.0.46 preserves FLOAT/DOUBLE negative and positive zero as separate set members: the reported UNION/INTERSECT/EXCEPT counts are 2/0/1, while MatrixOne returns 1/1/0. Use a set-only UINT64 equality key after common-type resolution and regenerate it during prepared specialization. Preserve original result representatives, existing NaN/scale normalization, PAD SPACE handling, global duplicate elimination and spill recovery. Scalar comparison, GROUP BY, SELECT DISTINCT, distinct aggregates and UNION ALL retain their existing equality domains.
Retaining independent UNION arms also preserves correlation cardinality, producer boundaries and separate prepared parameters previously hidden by textual identical-arm collapse. Proven aggregate/LIMIT boundaries reuse their original producer; supported EXISTS/IN paths retain existing predicate/NULL semantics. Selective predicates on unchanged integer/bool outputs can still reach both branches. The scalar fallback uses stream-occurrence identity plus existing WINDOW, LEFT/LoopJoin, aggregation and SINGLE owners to distinguish empty input, one NULL and multiple distinct members, including duplicate outer occurrences and mixed dependent expressions.
The fallback remains coordinator-local at DOP 1, with one shared statement-attempt account (256 MiB or the smaller inherited limit), a 16,777,216 candidate-pair budget and one of two CN permits. False predicates consume work; continuation yields do not refund it. Work/concurrency rejection returns terminal error 20833. A permit is released only after exact valid zero-live accounting; private initialization/cleanup failures keep uncertain owners out of reuse pools. Prepared attempts obtain fresh identity and leases.
Rebase onto official
7038c8520e5fc1ba456914b3c389ccedb69e22f7preserves VIEWS metadata at functions603/604 and MORPC126, LOAD IGNORE diagnostics at602/125, and TIMESTAMPADD SECOND at601/124. This PR's single set-key/scalar-marker capability advances to function605, sentinel606 and MORPC127. Node96 and Instruction65 are unchanged. Placement, destination/send/receive, actual-stream admission, native construction/Prepare, retained CTAS and persisted read/authoring gates agree. Missing or126 workers fall back to the capable coordinator and are rejected as actual set-key destinations before sending. Marked stages remain coordinator-local.Requirements compose independently: VIEWS-only streams require126, LOAD-only125; adding a set key or marker requires127. The new VIEWS/key/marker matrix covers unknown and124/125/126/127 capabilities, response release and actual-stream requirements; the existing stream-handshake test now includes127. Immediate-predecessor126 controls cover native/retained execution, CTAS Compile/Run and persisted admission. Earlier unpublished set-key601/602/603 fixtures remain historical, never aliases to released TIMESTAMPADD601, LOAD602 or VIEWS603/604.
The integration preserves stable view headings and expanded-star regeneration, shared-CTE publication and scalar column demand, prepared FIELD/CASE domains, ENUM/SET assignment domains and DUMP create-only publication. The RFC revision-26 receipt binds 456 source fingerprints, commands, terminal results, log hashes and evidence reuse:
Any()expectation is recorded as failed; the corrected complete protobuf plan suite passes, retaining VIEWS603/604, independent setkey605 and unknown606 assertions.Capacity and race qualification remains historical and source-bound:26 profiles,165 samples, cold first invocation separate, one excluded warm-up and three measured rounds. Runtime/account/lease owners retain their accepted limits; no new-head latency/RSS, external/APPLY capacity or distributed scalar qualification is claimed. Existing Darwin/arm64 native facilities are read-only test inputs. Native implementation/module inputs and protobuf marker fields are unchanged; no fresh native rebuild or Linux hardware/performance qualification is claimed.
The starting13d1 head has successful enabled CI checks and an independent implementation approval. This update resolves the new official allocation conflict; new-head CI is left to the push-triggered run. No old-run rerun, issue creation or duplicate feedback reply is part of this delivery. The issue should remain open after merge for reporter verification.