fix: scope cold Iceberg tables and watermark by PG schema#55
Merged
Conversation
📝 WalkthroughWalkthroughThe change makes Iceberg references and archival watermarks schema-aware, removes Iceberg namespace configuration, adds schema-qualified compactor input, makes partition configuration updates upserts, and aligns ColdFront tests, CI setup, examples, and documentation with the ChangesSchema-scoped archiving
Estimated code review effort: 4 (Complex) | ~45 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 | 0 |
| 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
🧹 Nitpick comments (1)
internal/view/view_test.go (1)
164-164: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAssert the table predicate too.
This assertion covers only half of the composite lookup. Add
table_name = 'events'so the test detects dropped or incorrectly bound table scoping.Suggested assertion
assert.Contains(t, sql, "schema_name = 'public'") // watermark lookup is schema-scoped +assert.Contains(t, sql, "table_name = 'events'")🤖 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/view/view_test.go` at line 164, Update the SQL assertion in the relevant view test to also require the table predicate table_name = 'events' alongside the existing schema_name check. Keep the assertion focused on validating both components of the schema-scoped watermark lookup.
🤖 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 `@docs/walkthrough.md`:
- Around line 81-83: Update the runnable namespace-seeding snippet in the
walkthrough to make repeated execution succeed: handle an already-existing
namespace by ignoring the duplicate response or checking for existence before
POSTing. Keep the existing bootstrap and warehouse retry behavior unchanged.
---
Nitpick comments:
In `@internal/view/view_test.go`:
- Line 164: Update the SQL assertion in the relevant view test to also require
the table predicate table_name = 'events' alongside the existing schema_name
check. Keep the assertion focused on validating both components of the
schema-scoped watermark lookup.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 11397183-372c-4766-8458-d490e942a70b
⛔ Files ignored due to path filters (24)
extension/coldfront/test/expected/allow_mixed_writes.outis excluded by!**/*.outextension/coldfront/test/expected/bakery_wraps_cold_writes.outis excluded by!**/*.outextension/coldfront/test/expected/cast_in_dquoted_identifier.outis excluded by!**/*.outextension/coldfront/test/expected/cast_literal_in_value.outis excluded by!**/*.outextension/coldfront/test/expected/cast_normalize.outis excluded by!**/*.outextension/coldfront/test/expected/classify_between_in_or.outis excluded by!**/*.outextension/coldfront/test/expected/classify_now.outis excluded by!**/*.outextension/coldfront/test/expected/cte_on_dml.outis excluded by!**/*.outextension/coldfront/test/expected/cte_on_insert.outis excluded by!**/*.outextension/coldfront/test/expected/cte_on_insert_1.outis excluded by!**/*.outextension/coldfront/test/expected/ddl_rename_view.outis excluded by!**/*.outextension/coldfront/test/expected/dollar_quote_in_value.outis excluded by!**/*.outextension/coldfront/test/expected/mixed_case_identifier.outis excluded by!**/*.outextension/coldfront/test/expected/param_cold_via_plpgsql.outis excluded by!**/*.outextension/coldfront/test/expected/param_cold_via_plpgsql_1.outis excluded by!**/*.outextension/coldfront/test/expected/returning_cold_rejected.outis excluded by!**/*.outextension/coldfront/test/expected/returning_literal_in_where.outis excluded by!**/*.outextension/coldfront/test/expected/schema_collision.outis excluded by!**/*.outextension/coldfront/test/expected/self_join_rejected.outis excluded by!**/*.outextension/coldfront/test/expected/update_ambiguous_rejected.outis excluded by!**/*.outextension/coldfront/test/expected/update_cold_via_view.outis excluded by!**/*.outextension/coldfront/test/expected/update_hot_via_view.outis excluded by!**/*.outextension/coldfront/test/expected/update_partition_key_blocked.outis excluded by!**/*.outextension/coldfront/test/expected/update_partition_key_move.outis excluded by!**/*.out
📒 Files selected for processing (50)
ci/journey.shci/lib.shci/probe-standby.shcmd/archiver/main.gocmd/archiver/main_test.gocmd/compactor/config.gocmd/compactor/config_test.gocmd/compactor/main.goconfig.example.yamldocs/architecture.mddocs/architecture_decoupled.mddocs/architecture_tiered.mddocs/compaction.mddocs/installation.mddocs/object_store.mddocs/usage.mddocs/walkthrough.mdexamples/walkthrough/config/archiver.yamlextension/coldfront/Makefileextension/coldfront/coldfront--1.0.sqlextension/coldfront/test/sql/allow_mixed_writes.sqlextension/coldfront/test/sql/bakery_wraps_cold_writes.sqlextension/coldfront/test/sql/cast_in_dquoted_identifier.sqlextension/coldfront/test/sql/cast_literal_in_value.sqlextension/coldfront/test/sql/cast_normalize.sqlextension/coldfront/test/sql/classify_between_in_or.sqlextension/coldfront/test/sql/classify_now.sqlextension/coldfront/test/sql/cte_on_dml.sqlextension/coldfront/test/sql/cte_on_insert.sqlextension/coldfront/test/sql/ddl_rename_view.sqlextension/coldfront/test/sql/dollar_quote_in_value.sqlextension/coldfront/test/sql/mixed_case_identifier.sqlextension/coldfront/test/sql/param_cold_via_plpgsql.sqlextension/coldfront/test/sql/returning_cold_rejected.sqlextension/coldfront/test/sql/returning_literal_in_where.sqlextension/coldfront/test/sql/schema_collision.sqlextension/coldfront/test/sql/self_join_rejected.sqlextension/coldfront/test/sql/update_ambiguous_rejected.sqlextension/coldfront/test/sql/update_cold_via_view.sqlextension/coldfront/test/sql/update_hot_via_view.sqlextension/coldfront/test/sql/update_partition_key_blocked.sqlextension/coldfront/test/sql/update_partition_key_move.sqlinternal/config/config.gointernal/config/config_test.gointernal/partcfg/commands.gointernal/partcfg/commands_test.gointernal/view/view.gointernal/view/view_test.gointernal/watermark/watermark.gointernal/watermark/watermark_test.go
💤 Files with no reviewable changes (5)
- internal/config/config_test.go
- examples/walkthrough/config/archiver.yaml
- ci/probe-standby.sh
- internal/config/config.go
- config.example.yaml
Comment on lines
81
to
83
| Bootstrap Lakekeeper, create the `wh` warehouse backed by SeaweedFS, | ||
| and seed the `default` namespace. The warehouse POST retries until | ||
| and seed the `public` namespace. The warehouse POST retries until | ||
| SeaweedFS is ready, and the namespace creation is idempotent: |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## docs/walkthrough.md excerpt\n'
nl -ba docs/walkthrough.md | sed -n '70,130p'
printf '\n## search for namespace creation references\n'
rg -n "namespace|public" -S docs src . --glob '!**/node_modules/**' --glob '!**/dist/**' --glob '!**/build/**' | sed -n '1,200p'Repository: pgEdge/coldfront
Length of output: 226
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## locate Lakekeeper namespace-create docs or API references\n'
rg -n "create.*namespace|namespace.*create|409|conflict|already exists|idempotent" -S . --glob '!**/node_modules/**' --glob '!**/dist/**' --glob '!**/build/**' | sed -n '1,240p'Repository: pgEdge/coldfront
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## maybe API routes / handlers mentioning namespaces\n'
fd -a -t f . .Repository: pgEdge/coldfront
Length of output: 11721
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## docs/walkthrough.md lines 70-130\n'
sed -n '70,130p' docs/walkthrough.md | cat -n
printf '\n## examples/walkthrough/setup.sh\n'
sed -n '1,220p' examples/walkthrough/setup.sh | cat -n
printf '\n## docs/usage.md namespace references\n'
rg -n "namespace|idempotent|already exists|409" docs/usage.md docs/architecture.md docs/installation.md docs/index.md -n -SRepository: pgEdge/coldfront
Length of output: 8170
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## docs/walkthrough.md relevant section\n'
sed -n '75,125p' docs/walkthrough.md
printf '\n## examples/walkthrough/README.md\n'
sed -n '1,220p' examples/walkthrough/README.md
printf '\n## examples/walkthrough/guide.sh relevant namespace lines\n'
rg -n "namespace|POST|curl|public|idempotent|409|already exists" examples/walkthrough/guide.sh examples/walkthrough/setup.sh examples/walkthrough/runner.sh examples/walkthrough/install.sh -SRepository: pgEdge/coldfront
Length of output: 15923
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## docs/walkthrough.md lines 75-125\n'
sed -n '75,125p' docs/walkthrough.md
printf '\n## examples/walkthrough/setup.sh lines 1-220\n'
sed -n '1,220p' examples/walkthrough/setup.sh
printf '\n## docs/usage.md lines around namespace setup\n'
sed -n '50,80p' docs/usage.mdRepository: pgEdge/coldfront
Length of output: 5889
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## focused search in walkthrough example files\n'
rg -n "curl|POST|namespace|warehouse|public|already exists|409|idempotent" \
docs/walkthrough.md examples/walkthrough/{README.md,guide.sh,setup.sh,runner.sh,install.sh} \
docs/usage.md docs/architecture.md \
-SRepository: pgEdge/coldfront
Length of output: 15049
Make the namespace seed rerunnable. The prose calls this idempotent, but the runnable snippet still uses a bare curl -sf POST with no duplicate handling, so a second run can fail on an existing namespace. Match this block to the guide script by ignoring the duplicate response or checking first.
🤖 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 `@docs/walkthrough.md` around lines 81 - 83, Update the runnable
namespace-seeding snippet in the walkthrough to make repeated execution succeed:
handle an already-existing namespace by ignoring the duplicate response or
checking for existence before POSTing. Keep the existing bootstrap and warehouse
retry behavior unchanged.
Collaborator
Author
|
@coderabbitai "out-of-scope": cause is linked so intentionally kept in this PR via |
|
Tip For best results, initiate chat on the files or code changes.
✏️ Learnings added
|
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
1 participant
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.
Each PG schema now maps to its own Iceberg namespace (
ice.<schema>.<table>), andarchive_watermarkis keyed by(schema, table), so two same-named tables in different schemas no longer share cold storage or a watermark. Also makesarchiver registeridempotent (upsert). Adds aschema_collisionregression test.Closes #53, #52.