Improve mtree build message on an empty table#148
Conversation
📝 WalkthroughWalkthroughChangesThe Merkle tree builder now handles unavailable and all-zero row estimates explicitly, and an integration test verifies the empty-table error message and pool cleanup behavior. Empty-table Merkle handling
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
|---|---|
| Complexity | 5 |
| Duplication | 2 |
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.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/consistency/mtree/merkle.go (1)
1532-1544: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRely on the deferred cleanup to close pools.
The explicit loops to close the pools before returning are redundant. The
deferregistered at the beginning of the function (line 1477) executes immediately upon anyreturn, ensuring all pools are closed safely without needing manual intervention on each error path.(You can also safely remove the identical loop around line 1510 for the same reason.)
♻️ Proposed refactor
if successfulEstimates == 0 { - for _, p := range pools { - p.Close() - } return fmt.Errorf("could not determine a reference node; failed to get row estimates from all nodes") } if maxRows == 0 { - for _, p := range pools { - p.Close() - } return fmt.Errorf("table %s has 0 rows on all nodes; add data before building a Merkle tree", m.QualifiedTableName) }🤖 Prompt for 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. In `@internal/consistency/mtree/merkle.go` around lines 1532 - 1544, Remove the explicit pool-closing loops before the returns for the successfulEstimates == 0 and maxRows == 0 checks, and also remove the identical loop around the earlier error path near line 1510. Rely on the defer registered at the start of the containing function to close all pools on every return.
🤖 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.
Nitpick comments:
In `@internal/consistency/mtree/merkle.go`:
- Around line 1532-1544: Remove the explicit pool-closing loops before the
returns for the successfulEstimates == 0 and maxRows == 0 checks, and also
remove the identical loop around the earlier error path near line 1510. Rely on
the defer registered at the start of the containing function to close all pools
on every return.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cf67fcb0-8aba-4774-a895-2451b648a4c6
📒 Files selected for processing (2)
internal/consistency/mtree/merkle.gotests/integration/mtree_empty_table_test.go
Improve mtree build message on an empty table
ACE-204