fix: mtree table-diff drops rows in the open-ended tail leaf#122
Conversation
GeneratePkeyOffsetsQuery always emits a final leaf whose range_end is
NULL — LEAD() over the trailing seq=2 row returns NULL — so the
"open-ended tail" leaf is the common case, not a corner one.
getPkeyBatches collected only non-NULL boundaries when building slices,
so the open-ended tail contributed no slice, and any row on the
non-reference node whose pkey sat beyond the reference's last_row was
never queried by processWorkItem.
The bug surfaces whenever a non-reference node holds rows outside the
reference's pkey envelope. In the original report (n1=[id=1],
n2=[id=2], both nodes 1 row each → n1 wins reference on tied counts)
the diff JSON reports n1=[{id:1}] and n2=[]; id=2 falls into the
[1, NULL) tail and is dropped. table-repair then deletes n1's row
instead of converging both nodes.
getPkeyBatches now tracks hasOpenStart / hasOpenEnd while collecting
finite boundaries and appends an extra {nil, firstBoundary} or
{lastBoundary, nil} slice on the open side(s); for the degenerate case
where every mismatched leaf is fully unbounded it emits a single
{nil, nil} fully-open slice. processWorkItem already treats nil bounds
as "no clause for this side", so the resulting SQL becomes
WHERE pkey >= lastBoundary (or <= firstBoundary), or WHERE TRUE for the
fully-open form, and finally sees the previously-invisible rows.
TestMerkleTreeBidirectionalDiff pins three scenarios that distinguish
this from a generic "diff is unidirectional" bug:
* ExactReproducer (n1=[1], n2=[2]) mirrors the report verbatim.
* N2HasRowsAboveN1Max stacks n2 extras above n1 max with n1 still the
reference; the open-ended-tail theory predicts n2 extras vanish from
the diff, which the pre-fix run confirmed exactly.
* N1HasRowAboveN2Max flips row counts so n2 becomes the reference; the
same bug surfaces mirrored on the n1 side, ruling out alternative
explanations that do not depend on the reference role.
The test also documents a setup gotcha learned while writing it:
spock.repair_mode(true) does NOT suppress replication for tables
freshly added to a Spock replication set in this environment. Future
mtree tests that need divergent per-node data should omit
spock.repset_add_table rather than rely on repair_mode.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 39 minutes and 7 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe PR fixes a merkle tree batch generation bug where open-ended leaf ranges (with NULL start or end boundaries) were not generating correct query coverage. The core fix reworks ChangesMerkle Tree Open-Ended Range Fix
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 |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Duplication | 0 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/consistency/mtree/merkle.go`:
- Around line 2198-2210: The loop that builds boundaries should treat "open"
bounds not only when RangeStart/RangeEnd is nil but also when they are empty
slices or slices containing only nils (as handled earlier at line ~1817); update
the loop in merkle.go that iterates allRanges to normalize each bound before
using it: for each r, determine isOpenStart/isOpenEnd by checking (r.RangeStart
== nil) OR (len(r.RangeStart) == 0) OR (allNil(r.RangeStart)) and likewise for
r.RangeEnd, set hasOpenStart/hasOpenEnd when open, otherwise append the
normalized bound to boundaries; reuse the existing allNil helper (or implement a
small check) so you don't append bogus []any{nil} entries.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7e2e73b9-07ce-42b1-8161-82469c5119e1
📒 Files selected for processing (2)
internal/consistency/mtree/merkle.gotests/integration/merkle_tree_test.go
Follow-up on bb94210. Two small cleanups: * When sortedBoundaries has exactly one element AND either hasOpenStart or hasOpenEnd, the single-point {b, b} slice is fully subsumed by the open-side slice appended just below ({nil, b} or {b, nil}). The redundant slice spawned a second work item that queried the same row on both nodes; addRowToDiff masks the duplication in the output but the extra round-trip per worker is pure waste. Gate the case-1 emit on neither side being open. * Trim the verbose explanatory comments to a single tight block. The commit body of bb94210 already documents the mechanism end-to-end; inline prose only needs to flag the non-obvious why. Also removes a copy-paste typo in the deleted open-side comment ("firstBoundary" used twice where the open-end half should have read "lastBoundary"). No behaviour change in the test suite: mtree integration (simple PK, composite PK, --until, bidirectional) still all green.
2 participants
Summary
ace mtree table-diffsilently dropped any row on the non-reference node whose pkey sat past the reference'slast_row.GeneratePkeyOffsetsQueryemits a final leaf withrange_end = NULL, andgetPkeyBatchesignored NULL bounds when building work-item slices — the open-ended tail produced no query. Withn1=[id=1], n2=[id=2](the bug report) the diff returnedn1=[{id:1}], n2=[];table-repairthen deleted n1's row instead of converging.Fix
getPkeyBatchesnow trackshasOpenStart/hasOpenEndand appends{nil, firstBoundary}or{lastBoundary, nil}on the open side(s); the fully-unbounded edge case emits{nil, nil}.processWorkItemalready treats a nil bound as "no clause for this side", so no caller changes. The single-point{b, b}slice is skipped when an open-side slice would subsume it.