Add --until support to mtree, fix frozen row handling#110
Merged
mason-sharp merged 1 commit intomainfrom Apr 16, 2026
Merged
Conversation
📝 WalkthroughWalkthroughAdds an RFC3339Nano Changes
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
TIP This summary will be updated as you push new changes. Give us feedback
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
internal/consistency/mtree/merkle.go (1)
1131-1137: Clear staleuntilTimebefore parsing.If the same
MerkleTreeTaskinstance is reused andUntilis later cleared, the old parsed cutoff remains inm.untilTimeand silently affects the next run.Suggested change
+ m.untilTime = nil if trimmed := strings.TrimSpace(m.Until); trimmed != "" { + m.Until = trimmed parsed, err := time.Parse(time.RFC3339, trimmed) if err != nil { return fmt.Errorf("invalid value for --until (expected RFC3339 timestamp): %w", err) } m.untilTime = &parsed }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/consistency/mtree/merkle.go` around lines 1131 - 1137, Clear any previous parsed cutoff before evaluating the current Until value: in the MerkleTreeTask handling code (MerkleTreeTask, fields Until and untilTime) set m.untilTime = nil before trimming/parsing; then if trimmed != "" parse into parsed and set m.untilTime = &parsed, otherwise leave it nil so an earlier value won't persist across reused instances. Ensure this happens prior to returning on parse error so a cleared Until doesn't retain stale state.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@db/queries/queries.go`:
- Around line 54-58: CommitTimestampFilter currently formats the cutoff using
time.RFC3339 which drops fractional seconds; update the function
(CommitTimestampFilter) to format the time with sub-second precision (e.g.
time.RFC3339Nano) so timestamps like 2026-04-15T12:00:00.500Z keep their
fractional part when interpolated into the SQL predicate.
In `@internal/consistency/mtree/merkle.go`:
- Line 1342: The build path applies CommitTimestampFilter only to
GeneratePkeyOffsetsQuery, but computeLeafHashes and the maxRows estimate still
read the live table; propagate the same CommitTimestampFilter(m.untilTime) into
the code paths used by computeLeafHashes and the row-count/estimate logic so
that leaf hashing and maxRows use the same as-of predicate; specifically, ensure
the functions/methods that produce the SQL for computeLeafHashes and any maxRows
calculation accept and apply the CommitTimestampFilter (or reuse the
offsetsQuery's filter) so the persisted tree represents the snapshot as of
m.untilTime.
In `@tests/integration/merkle_tree_test.go`:
- Around line 110-128: The VACUUM FREEZE call can freeze a post-fence divergent
tuple and invalidate the intent of the test; move the VACUUM FREEZE of
qualifiedTableName on both pools so it runs before the fence and before creating
the node-local update (i.e., freeze the shared baseline earlier), ensuring
newTestMerkleTreeTask / taskAfterFreeze observes a pre-fence frozen baseline
rather than freezing the divergent tuple created after the fence.
In `@tests/integration/table_diff_test.go`:
- Around line 115-131: The VACUUM FREEZE is being run after the post-fence
divergent update, which can allow the divergent tuple to be frozen and change
test behavior; instead, run VACUUM FREEZE on both nodes before creating the
divergent row: call the VACUUM FREEZE loop prior to capturing fence and before
performing the post-fence update, then set taskAfterFreeze :=
newTestTableDiffTask(...), set taskAfterFreeze.Until = fence.Format(...), and
run RunChecks/ExecuteTask to assert diffs; this ensures the baseline is frozen
first and the test proves frozen pre-fence rows remain visible.
---
Nitpick comments:
In `@internal/consistency/mtree/merkle.go`:
- Around line 1131-1137: Clear any previous parsed cutoff before evaluating the
current Until value: in the MerkleTreeTask handling code (MerkleTreeTask, fields
Until and untilTime) set m.untilTime = nil before trimming/parsing; then if
trimmed != "" parse into parsed and set m.untilTime = &parsed, otherwise leave
it nil so an earlier value won't persist across reused instances. Ensure this
happens prior to returning on parse error so a cleared Until doesn't retain
stale state.
🪄 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: eeb6b225-64ee-4926-9917-7355b9fa07ce
📒 Files selected for processing (8)
.github/workflows/test.ymldb/queries/queries.godb/queries/queries_test.gointernal/cli/cli.gointernal/consistency/diff/table_diff.gointernal/consistency/mtree/merkle.gotests/integration/merkle_tree_test.gotests/integration/table_diff_test.go
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 `@internal/consistency/mtree/merkle.go`:
- Around line 1131-1137: The validation logic for MerkleTreeTask leaves
m.untilTime set from previous runs when m.Until is empty; update the block that
checks strings.TrimSpace(m.Until) in the MerkleTreeTask validation (the code
handling m.Until and m.untilTime) so that when trimmed == "" you explicitly set
m.untilTime = nil to clear any stale value, and keep the existing parsing/err
handling when trimmed is non-empty (i.e., only parse and set m.untilTime on
non-empty input and clear it otherwise).
🪄 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: 48475af6-65f6-4e51-afcc-e12bb7f2e893
📒 Files selected for processing (6)
db/queries/queries.gointernal/cli/cli.gointernal/consistency/diff/table_diff.gointernal/consistency/mtree/merkle.gotests/integration/merkle_tree_test.gotests/integration/table_diff_test.go
✅ Files skipped from review due to trivial changes (2)
- internal/cli/cli.go
- tests/integration/merkle_tree_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- db/queries/queries.go
- internal/consistency/diff/table_diff.go
- tests/integration/table_diff_test.go
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/integration/merkle_tree_test.go (1)
109-109: Prefer a single config-derived block size instead of repeating1000.Using a shared test-local value derived from the configured minimum block size will keep this test stable if
min_block_sizechanges.Also applies to: 145-145, 162-162
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/merkle_tree_test.go` at line 109, Replace the hard-coded literal 1000 used to set mtreeTask.BlockSize (and the same literal occurrences at the other noted lines) with a test-local constant derived from the app configuration's minimum block size (e.g., read from min_block_size or Config.MinBlockSize in tests), so the test uses a single shared value like testBlockSize = max(1, Config.MinBlockSize) and then assign mtreeTask.BlockSize = testBlockSize; update the other two occurrences to reference that same testBlockSize constant to keep the test stable if min_block_size changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/integration/merkle_tree_test.go`:
- Around line 123-128: Update the stale block comment in merkle_tree_test.go to
accurately describe the actual operation performed (an UPSERT on id=5) rather
than describing an “insert a new row” with alternative candidate IDs; locate the
test code that issues the UPSERT (the statement updating/inserting id=5 in the
Merkle tree integration test) and rewrite the comment to state that the test
performs an upsert on an existing key (id=5) to simulate a divergence, removing
the misleading suggestions about deleting or inserting other IDs.
---
Nitpick comments:
In `@tests/integration/merkle_tree_test.go`:
- Line 109: Replace the hard-coded literal 1000 used to set mtreeTask.BlockSize
(and the same literal occurrences at the other noted lines) with a test-local
constant derived from the app configuration's minimum block size (e.g., read
from min_block_size or Config.MinBlockSize in tests), so the test uses a single
shared value like testBlockSize = max(1, Config.MinBlockSize) and then assign
mtreeTask.BlockSize = testBlockSize; update the other two occurrences to
reference that same testBlockSize constant to keep the test stable if
min_block_size changes.
🪄 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: 92878495-34bb-4c10-87bc-323e74dc8b57
📒 Files selected for processing (3)
internal/consistency/diff/table_diff.gointernal/consistency/mtree/merkle.gotests/integration/merkle_tree_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/consistency/diff/table_diff.go
- internal/consistency/mtree/merkle.go
…n tests Add --until flag to mtree build/diff commands, extract shared CommitTimestampFilter helper that handles frozen rows (NULL commit timestamps after VACUUM FREEZE), and add integration tests covering both table-diff and mtree paths including post-freeze verification. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
mason-sharp
force-pushed
the
mtree-until
branch
from
0864126 to
203d932
Compare
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 `@tests/integration/merkle_tree_test.go`:
- Around line 78-82: The cleanup closure currently ignores errors from pool.Exec
and can leak state; update the t.Cleanup anonymous function to capture the Exec
error for each pool (e.g., use _, err := pgCluster.Node1Pool.Exec(ctx,
fmt.Sprintf(...)) and likewise for Node2Pool) and handle non-nil errors by
failing the test (t.Fatalf) or at minimum logging them (t.Errorf/t.Logf) with
context including which pool and the error and the qualifiedTable; ensure both
Exec calls are checked separately so any failure is surfaced.
🪄 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: c5d89933-4a31-4bad-91ce-31e42981250c
📒 Files selected for processing (8)
.github/workflows/test.ymldb/queries/queries.godb/queries/queries_test.gointernal/cli/cli.gointernal/consistency/diff/table_diff.gointernal/consistency/mtree/merkle.gotests/integration/merkle_tree_test.gotests/integration/table_diff_test.go
✅ Files skipped from review due to trivial changes (2)
- db/queries/queries_test.go
- db/queries/queries.go
🚧 Files skipped from review as they are similar to previous changes (5)
- .github/workflows/test.yml
- internal/cli/cli.go
- tests/integration/table_diff_test.go
- internal/consistency/mtree/merkle.go
- internal/consistency/diff/table_diff.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add --until flag to mtree build/diff commands, and handle frozen rows (NULL commit timestamps) in the shared CommitTimestampFilter.
ACE-162