fix(frontend): preserve non-finite data branch floats - #26504
Conversation
daa6619 to
1fd0b38
Compare
1fd0b38 to
0d7238e
Compare
XuPeng-SH
left a comment
There was a problem hiding this comment.
Deep-reviewed updated exact head 8e852a000875b8811b6b6bfee06d837ae2d6e350. The exact storage-identity fix closes MERGE and portable-output paths, but one original-issue path still bypasses it.
P1 — FLOAT/DOUBLE primary-key updates still fail through DATA BRANCH PICK
mergeDiffs and portable output call appendBatchRowsAsSQLValues, which recognizes fromUpdate and emits the new exact-key UPDATE. pickMergeDiffs, however, calls appendPickedBatchRows; that function ignores fromUpdate and still queues the pair as staged DELETE plus replacement INSERT. This is precisely the delete/reinsert cycle that the PR correctly identifies as unsafe for FLOAT/DOUBLE keys.
I reproduced it through public SQL on a service built from this exact head. The base/source/destination contain four legal, bit-distinct DOUBLE primary keys: two NaN payloads and both signed zeros. The source changes only NaN payload 1 and negative zero, then:
DATA BRANCH PICK src INTO dst
KEYS(SELECT k FROM src WHERE note IN ('nan1_updated', 'negzero_updated'))
WHEN CONFLICT ACCEPT;
returns:
ERROR 1062: Duplicate entry '0' for key 'k'
The destination transaction rolls back and retains all four original rows. Ordinary MERGE on the same representations is covered by the PR, but PICK is explicitly part of #26072 and remains broken for a real FLOAT/DOUBLE key update.
Please route PICK same-key updates through exact-key UPDATE while preserving its FAIL/SKIP/ACCEPT conflict semantics. Add public PICK regressions with bit-distinct NaN payloads and signed zero, plus a composite float-key control; verify both updated and untouched encodings with serial(...).
|
XuPeng-SH's PICK review is valid and in scope. Fixed on rebased head PICK now uses the same shared exact FLOAT/DOUBLE update classification and row application as MERGE/portable output, but only after its existing FAIL/SKIP/ACCEPT decision. Marked delete rows still drive conflict handling and are then suppressed; accepted marked inserts execute an exact-key Public regressions cover FLOAT, DOUBLE, and composite FLOAT/DOUBLE/int PICK updates with two NaN payloads and both signed zeros. The untouched payload-0/+0 rows remain, only payload-1/-0 rows change, and |
XuPeng-SH
left a comment
There was a problem hiding this comment.
Deep-reviewed exact head 74cef706ec05471643afb24102edc3c034648e48. The previous PICK case with an unchanged destination is fixed, but the UPDATE-only application path introduces a silent-loss case when the destination no longer contains the source key.
P1 — ACCEPT loses a FLOAT/DOUBLE source update when the destination deleted the row
dataBranchExactFloatKeyUpdateBatch classifies both halves of every same-key update on a FLOAT/DOUBLE-containing PK as an exact update. The consumer then suppresses the DELETE half, and appendOrExecuteDataBranchApplyRow turns the INSERT half into only:
UPDATE dst SET ... WHERE serial(pk) = serial(source_pk) LIMIT 1If the destination branch independently deleted that row, the UPDATE affects zero rows. There is no INSERT fallback, so WHEN CONFLICT ACCEPT reports success but does not materialize the accepted source row.
I reproduced this through public SQL on a service built from this exact head:
CREATE TABLE base_t(k DOUBLE PRIMARY KEY, note VARCHAR(32));
INSERT INTO base_t VALUES (1.5, 'base');
DATA BRANCH CREATE TABLE src FROM base_t;
DATA BRANCH CREATE TABLE dst FROM base_t;
UPDATE src SET note = 'source_updated' WHERE k = 1.5;
DELETE FROM dst WHERE k = 1.5;
DATA BRANCH PICK src INTO dst
KEYS(SELECT k FROM src WHERE note = 'source_updated')
WHEN CONFLICT ACCEPT;
SELECT COUNT(*) FROM dst; -- 0, expected 1The FAIL control correctly reports this pair as a conflict, proving ACCEPT is taking the wrong resolution rather than seeing a no-op. The same witness with an INT PK restores the source row as expected. I also reproduced the zero-row result for:
- FLOAT through PICK;
- DOUBLE through MERGE;
- a composite
(FLOAT, INT)PK through MERGE.
The portable path shares the same UPDATE-only helper, so its generated script has the same missing-row state unless it is handled separately.
Please preserve exact float-key identity while closing the complete conflict state: after ACCEPT, the source row must exist whether the destination retained, modified, or deleted the ancestor row. Add public PICK and MERGE regressions for destination-delete versus source-update across FLOAT, DOUBLE, and a composite float key, with FAIL/SKIP controls; also round-trip the portable script and retain the existing NaN-payload/signed-zero identity assertions.
Fresh exact-head evidence: go build and go vet for pkg/frontend pass; the full pkg/frontend suite passes; the focused float-key regressions pass under -race -count=3. Those tests currently cover only the destination-retained branch of this state machine.
|
XuPeng-SH's destination-delete review is valid and in scope. Fixed on rebased head |
XuPeng-SH
left a comment
There was a problem hiding this comment.
Blocking correctness issue in pkg/frontend/data_branch_output.go:2055: the new restore path uses INSERT ... SELECT, even though lines 232-236 document that INSERT ... SELECT collapses bit-distinct FLOAT/DOUBLE keys. Reproduced on exact head ae748dc: independently insert +0 and -0 into a DOUBLE PK table; update the exact -0 row on the source branch; delete exact -0 on the destination while +0 remains; then MERGE ... WHEN CONFLICT ACCEPT fails with Error 1062: Duplicate entry -0. PICK ... WHEN CONFLICT ACCEPT fails identically. The serial-based NOT EXISTS sees -0 as missing, but the INSERT ... SELECT path then compares with scalar float semantics and collides with +0. Portable output emits the same statement. Please restore missing exact-key updates through a path that preserves independent single-row INSERT semantics, and add combined missing-key/bit-distinct-peer regressions (+0/-0 and distinct NaN payloads) for MERGE, PICK, and portable output.
|
XuPeng-SH's review is valid and in scope. Fixed on rebased head The review correctly identified that The clean fix closes both boundaries:
Public regressions combine a missing key with its untouched bit-distinct peer for both +0/-0 and two NaN payloads across MERGE, PICK, and portable SQL. All four exact Verification on base |
|
The proxy BVT failure on ad38897 was valid and PR-caused. The exact FLOAT/DOUBLE identity expression was correctly serialized for DEDUP, but duplicate diagnostics printed the binary key, producing the ALTER TABLE expectation mismatch; Coverage only propagated that producer failure. Fixed on rebased head 0570eb7 by decoding exact keys for errors and closing the same projected-type/serial(pk) contract in fuzzy-filter collision recovery. Rebased onto current main bb011f2 with no conflicts. Controlled tests/vet, focused race x3, make build, diff checks, DDL BVT 923/923 + 764/764, and Data Branch BVT 212/212 pass. The PR description is rewritten and current. |
|
The Coverage failure on |
|
The Coverage failure on |
XuPeng-SH
left a comment
There was a problem hiding this comment.
The previous INSERT ... SELECT blocker is fixed, but the hash-diff conflict join still collapses bit-distinct FLOAT/DOUBLE primary keys.
handleTarDeleteAndUpdates sorts each side by the raw PK vector (mergeutil.SortColumnsByIndex) and then treats compareSingleValInVector(...) == 0 as the same key. Both paths use scalar float ordering/comparison: +0 and -0 compare equal, and distinct NaN payloads compare equal. A minimal white-box witness on this head expects non-zero for compareSingleValInVector(+0, -0) but gets 0.
This leaves an uncovered cross-change shape. For example, start with both +0 and -0 as DOUBLE PKs; update -0 on the source while independently deleting +0 on the destination, then MERGE/PICK with conflict handling. These are different storage keys, but the merge join pairs them as a conflict. Under ACCEPT, lines 1188-1196 can mark the source -0 update as restoreMissing; the new direct INSERT then collides with the still-existing -0 row. FAIL/SKIP can likewise reject or skip a non-conflict. The same shape exists for two NaN payloads.
Please make hash-diff sorting and equality share one exact, bit-preserving key order (including composite keys), and add crossed-key regressions for +0/-0 and distinct NaN payloads across MERGE/PICK and FAIL/SKIP/ACCEPT. The current tests delete the same exact key that the source updates while merely retaining a bit-distinct peer, so they do not exercise this pairing bug.
|
XuPeng-SH’s hash-diff review is valid and in scope. Fixed in The conflict merge join now uses one shared exact key identity for both ordering and equality. Simple FLOAT/DOUBLE PKs are ordered by their raw IEEE bits, so signed zero and distinct NaN payloads remain separate storage keys; non-float keys retain the existing comparator, and composite PKs retain their existing serialized-byte order. Both sides are sorted through the same helper before the merge join, eliminating the former scalar-order mismatch. Regression coverage now crosses independent changes: source updates -0 while destination deletes +0, and source updates NaN payload 1 while destination deletes payload 0. MERGE and PICK cover FAIL/SKIP/ACCEPT, with a composite control and exact |
XuPeng-SH
left a comment
There was a problem hiding this comment.
Deep-reviewed the updated exact head. The previous hash-diff blocker is closed.
The conflict matcher now derives a raw IEEE-bit identity for a simple FLOAT/DOUBLE PK, sorts both sides by that uint64 identity, and uses the identical comparison in the merge join. This distinguishes +0 from -0 and distinct NaN payloads while leaving non-float keys unchanged; composite keys continue to use their existing exact serialized-byte identity. The temporary identity vector is batch-scoped and freed after sorting, so the extra O(n), 8-byte/row allocation is limited to simple float-key conflict batches.
The new public regressions cover the previously missing crossed-change shape: source updates -0/NaN payload 1 while destination deletes +0/payload 0, across MERGE and PICK with FAIL/SKIP/ACCEPT, plus a composite control and exact serial(...) assertions. This closes the false-conflict and erroneous restore/duplicate path without weakening real conflict handling.
Fresh exact-head validation passed: diff check; go list, go build, go vet, and the full pkg/frontend suite; focused exact-identity/sort tests under -race -count=3. CI is green.
Merge Queue Status
This pull request spent 1 hour 4 minutes 39 seconds in the queue, including 1 hour 4 minutes 14 seconds running CI. Required conditions to merge
|
XuPeng-SH
left a comment
There was a problem hiding this comment.
Revalidated the Mergify queue merge head 6f72fe538464. Its first parent is the reviewed/fixed head 12db1fb854; its second parent is current main 2190897330. The Data Branch, colexec, compile, plan, and float-special-value test patch is byte-identical to the approved parent. The only upstream delta from the first parent is pkg/logservice/service_bootstrap_test.go, and the merge result is exactly the current-main version—there is no conflict rewrite in this PR surface.
The exact float identity/sort tests pass on this merge head, and git diff --check is clean. Approval stands.
What type of PR is this?
Which issue(s) this PR fixes:
Fixes #26072
What this PR does / why we need it:
Root cause
Data Branch converts rows back into SQL for MERGE, PICK, and portable output. The shared float formatter emitted bare
NaN,+Inf, and-Inftokens, which the parser treated as expressions rather than FLOAT/DOUBLE values.Generated SQL and conflict matching also lost MatrixOne's bit-preserving FLOAT/DOUBLE storage identity:
cast(-0 as double)produces positive zero.Changes
bit_cast(unhex(...))literals.serial(...)as the shared exact identity for FLOAT/DOUBLE key components in LCA probes, staged deletes, portable predicates, MERGE, and PICK. Non-float key behavior is unchanged.serial(pk), and duplicate errors decode the original FLOAT/DOUBLE value.Tests
Verification on head
12db1fb854, based onc58cd292d3:pkg/frontend,pkg/sql/plan,pkg/sql/colexec, andpkg/sql/compile— pass.go vetfor those four packages — pass.make build— pass.git diff --check— pass.branch_float_special_values.sqlregression on this exact head — 256/256 statements pass.alter_table_change_column.sql923/923 andalter_table_modify_column.sql764/764 statements pass.The public Data Branch regression covers FLOAT, DOUBLE, and composite keys; finite values, infinities, signed zero, and distinct NaN payloads; MERGE, PICK, portable SQL, and no-PK predicates; FAIL/SKIP/ACCEPT when source update conflicts with destination delete; and crossed independent updates/deletes on bit-distinct signed-zero and NaN keys. Exact
hex(serial(...))assertions verify both changed and untouched keys.CI follow-up
The failed proxy BVT on head
ad38897235was caused by this PR: exact serialized FLOAT/DOUBLE DEDUP identities leaked into duplicate-entry messages in ALTER TABLE cases. This head decodes those identities for diagnostics and closes the same identity contract in fuzzy-filter collision recovery. That run's Coverage failure only propagated the producer failure.The later Coverage-only failures were also PR-caused. Head
0570eb78b8reported 178/240 (74.17%); after the first test-only follow-up, CI's merged profile one91247f83areported 175/240 (72.92%) because the background fuzzy-check path remained uncovered. The matching executor/result boundary test adds 14 covered statements, taking the observed 175/240 baseline above the strict 75% gate.Unrelated Ubuntu/x86 failures remain tracked in #26583 and #26632 through the single machine-readable blocker comment; this PR contains no workaround for them.
Residual risk
#26432 still tracks the engine-wide resident/spill typed-key inconsistency. This PR does not change that general engine contract. Data Branch and simple FLOAT/DOUBLE primary-key constraint paths avoid raw float identity matching by using exact bit/serialized encodings. Non-float paths are unchanged.