Fix batch-4 review findings: structural pull loss, invalid function SQL, view deps, and gate coverage#56
Merged
Merged
Conversation
COMMENT ON TRIGGER trg ON tbl already parsed correctly in ddl/object.rs (target.schema carries the owning table's qualified name, target.name the trigger name), but pull::Assembly::apply_comment had no TRIGGER arm, so the comment fell into the `_ => false` case and was silently discarded. Add apply_trigger_comment, mirroring the existing apply_column_comment two-name lookup, and a unit test asserting the comment lands on the trigger. RULE/POLICY/CONSTRAINT ... ON ... comments are deferred: their ddl-side parsing shares the same two-name shape (POLICY already has a ddl test), but there is no models::Trigger-equivalent field to attach a POLICY/RULE comment to without further model changes, which is out of scope here. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
CREATE TABLE x OF type (col WITH OPTIONS ..., ...) uses a distinct element-list production (OptTypedTableElementList/TypedTableElement), not TableElement, so create_table's single TableElement walk never saw these columns/constraints and silently dropped them. TypedTableElement is columnOptions (a ColId + ColQualList of the same ColConstraintElem/ ColConstraint shapes the plain column() parser already understands) or TableConstraint (same as an inline table-level constraint). Add a second walk over TypedTableElement in create_table (src/ddl/table.rs) that feeds columnOptions nodes straight into the existing column() helper (data_type stays empty, matching build::render_typed_table_column which never emits it) and TableConstraint nodes into the existing table_constraint()/apply_constraint() path. Added parses_typed_table_inline_column_constraints and parses_typed_table_check_constraint unit tests. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
dump_function's fallback branch used the bare d.name for func_name when a function had no structured `parameters`, which produced `CREATE FUNCTION schema.foo RETURNS ...` (missing `()`) for zero-argument functions that store a plain, unparenthesized name — invalid SQL that also broke the matching DROP FUNCTION and, since the default COMMENT ON FUNCTION target derives from the same name, COMMENT ON FUNCTION. Names that already carry an embedded `(argtypes)` signature (the on-disk convention used to disambiguate overloads) are left untouched, so parameterized functions render exactly as before. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
pg_dump emits column defaults (SERIAL's nextval(...), and any other
DEFAULT) as a standalone `ALTER TABLE ONLY t ALTER COLUMN c SET
DEFAULT ...` statement (libpgdump ObjectType::Default), not inline on
the CREATE TABLE column def. alter_table() only recognized `kw_add`
commands, so these fell through to Statement::Unsupported and the
default was silently lost — SERIAL columns pulled with no default at
all.
- src/ddl/mod.rs: add Statement::SetColumnDefault { table, column,
default }.
- src/ddl/table.rs: alter_table() now also matches
alter_table_cmd -> alter_column_default (kw_set + kw_default +
a_expr; kw_drop + kw_default is left unhandled, same as before).
Added parses_alter_table_set_column_default.
- src/pull/mod.rs: OT::Default is now routed through the parser (was
falling into push_remaining); Assembly::apply folds the default onto
the matching column of the already-ingested table, with a
deferred_defaults retry list (mirroring deferred_indexes/
deferred_partitions) for the case where the ALTER precedes its
table. Added set_default_attaches_to_existing_column and
deferred_default_attaches_after_table_is_ingested.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Views got no dependency edges (only foreign-key-derived table edges existed), so build ordered them purely by libpgdump's same-priority alphabetical sort — a view could be emitted before a table it queries, breaking restore. pg_dump's archive TOC does carry per-entry dump-id dependencies, but wiring that through would require plumbing the raw libpgdump::Dump (and its dump-id → object mapping, built in ingest()) into the writer, touching src/pull/mod.rs which another pass owns. Instead, re-parse each view/matview's stored query text with tree-sitter-postgres and walk the CST for schema-qualified relation_expr nodes, matching them against the pulled inventory (tables/views/materialized_views). Unqualified references (CTE names, or anything pg_dump left unqualified) and references outside the inventory are skipped rather than guessed at, and self-references are guarded against. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
pg_dump does not emit `CREATE TABLE child PARTITION OF parent FOR VALUES ...`; it emits the child as a plain `CREATE TABLE` followed by a separate `TABLE ATTACH` TOC entry (`ALTER TABLE ONLY parent ATTACH PARTITION child FOR VALUES ...`). The parser only recognized the inline `PARTITION OF` form, and the ingest dispatch had no arm for the `TABLE ATTACH` object type, so partition membership was silently lost on round-trip: the child surfaced as a standalone table and the parent had no partitions. Parse the `partition_cmd` ATTACH form into a new `Statement::AttachPartition`, route `ObjectType::TableAttach` through the parser, and fold attached children into their parent during pull assembly — moving the bounds (and any comment picked up on the child table) into the parent's `partitions` and dropping the child's standalone table, mirroring the existing inline-`PARTITION OF` fold. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…' and 'fix/view-dependencies' into fix/review-findings-batch4
The RANGE-partition branch of the `partitions` oneOf required the misspelled key `for_vlaues_to` instead of `for_values_to`, so no RANGE partition (with `for_values_from`/`for_values_to`) could ever satisfy the schema — project load rejected every range-partitioned table. This was carried over verbatim from the Python schemata and only surfaced now that a fixture exercises range partition children. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Two invalid-SQL bugs in the built archive, both surfaced by round-tripping a fixture through pg_restore: 1. CREATE/DROP FUNCTION rendered a bare, unqualified function name. pg_restore runs with search_path reset to '' and schema-qualifies every object, so a bare `CREATE FUNCTION name(...)` fails with "no schema has been selected to create in". Render the function reference schema-qualified (the COMMENT target was already qualified). Python emitted the bare form too, so the three test-project functions move into the documented build_parity deviations. 2. A SERIAL column's `nextval(...)` default was inlined into CREATE TABLE, but the sequence is OWNED BY that column: inlining makes the table depend on the sequence while the sequence depends on the table, an unrestorable cycle. Mirror pg_dump by holding the default out of CREATE TABLE and emitting it as a separate ALTER TABLE ... SET DEFAULT entry that depends on both the table and the sequence. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Deploy mapped each archive entry to its owning inventory item by looking one dependency level deep. A COMMENT ON TRIGGER (and COMMENT ON INDEX) depends on the TRIGGER/INDEX entry, which is itself a child of the table rather than a modeled inventory item, so its owner set resolved empty and the entry was silently dropped: deploying a fresh table with a commented trigger lost the comment (caught by the equivalence gate). Walk the dependency graph through intermediate non-item entries until it reaches inventory items. This only recovers entries that were previously skipped for having no resolvable owner, so it cannot change any entry that already mapped to an item. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Re-enable the objects batch 3 documented but had to leave out because the underlying bugs made them fail. Each now exercises a fix through the round-trip and deploy gates: - partition children via ATTACH PARTITION (pull fold) - a typed table with an inline column constraint - a zero-argument trigger function (build's `()` + schema qualification) - a trigger with a COMMENT ON TRIGGER (pull capture + deploy emission) - a view ordered alphabetically before its table (view dependency edges) - a SERIAL column (separate SET DEFAULT capture + build split) The trigger function body is authored in libpgfmt's normalized two-space form so the round-trip schema diff stays clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR adds parsing and round-trip support for attached partitions, column defaults, typed-table constraints, trigger comments, and zero-arg functions. It also splits sequence-backed defaults during build, adds view dependency metadata, updates deploy owner resolution, and refreshes fixtures and parity expectations. ChangesRound-trip and planning updates
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Ingest
participant Assembly
participant ParentTable
participant ChildTable
Ingest->>Assembly: parse OT::Default and OT::TableAttach
Assembly->>Assembly: buffer deferred defaults and attached partitions
Assembly->>Assembly: finalize ingest
Assembly->>ParentTable: apply_deferred_defaults()
Assembly->>ChildTable: apply_attached_partitions()
Assembly->>ParentTable: attach trigger comment and partition metadata
sequenceDiagram
participant Builder
participant TableDump
participant FunctionDump
participant ArchiveEntries
Builder->>Builder: split_sequence_defaults()
Builder->>TableDump: omit inline nextval defaults
Builder->>ArchiveEntries: emit separate DEFAULT entry
Builder->>FunctionDump: compute func_name and comment_target
FunctionDump->>ArchiveEntries: add_item_with_comment_target()
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![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 (2)
src/pull/writer.rs (1)
51-85: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider deduplicating the view/materialized-view serialization loops.
The two loops (Lines 52-67 and 68-85) are identical except for the output path's plural key (
"views"vs"materialized_views"). Extracting a small helper keeps the dependency-injection logic from silently diverging if it's later edited in only one place.♻️ Optional helper extraction
let mut save_view = |view: &models::View, kind: &str| -> Result<(), String> { let mut value = serialize(view)?; if let (Some(map), Some(dependencies)) = ( value.as_object_mut(), view_dependencies( &view.schema, &view.name, view.query.as_deref(), &relation_inventory, ), ) { map.insert(String::from("dependencies"), dependencies); } writer.save_value(nested(kind, &view.schema, &view.name)?, &value) }; for view in &assembly.views { save_view(view, "views")?; } for view in &assembly.materialized_views { save_view(view, "materialized_views")?; }(Type/borrow details may need adjustment to satisfy the borrow checker against
writer.)🤖 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 `@src/pull/writer.rs` around lines 51 - 85, The serialization logic for views and materialized views is duplicated in the writer flow, so extract the shared behavior into a small helper or closure around serialize, view_dependencies, and writer.save_value. Reuse that helper for both assembly.views and assembly.materialized_views, passing only the plural key ("views" or "materialized_views") and preserving the dependency injection into the serialized value. Make sure the helper works cleanly with writer and the borrow checker.src/pull/mod.rs (1)
1098-1129: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate relation-splitting logic between column and trigger comment handlers.
apply_column_commentandapply_trigger_commentboth splittarget.schemaon.into(schema, table)and resolve the owning table identically. Consider extracting a small shared helper (e.g.resolve_relation(target) -> Option<&mut models::Table>) to avoid the duplication.♻️ Proposed extraction
fn resolve_owning_table( &mut self, target: &QualifiedName, ) -> Option<&mut models::Table> { let relation = target.schema.as_ref()?; let (schema, table) = match relation.split_once('.') { Some((schema, table)) => (Some(schema.to_string()), table), None => (None, relation.as_str()), }; self.find_table(&QualifiedName { schema, name: table.to_string() }) }Also applies to: 1131-1164
🤖 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 `@src/pull/mod.rs` around lines 1098 - 1129, Both apply_column_comment and apply_trigger_comment duplicate the same target.schema split-and-table lookup logic, so extract that resolution into a shared helper in this module (for example, resolve_owning_table or similar) and have both comment handlers call it before updating the column or trigger. Keep the helper responsible for splitting the relation into schema/table and calling find_table on a QualifiedName so the two handlers only contain their specific comment assignment logic.
🤖 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 `@src/ddl/table.rs`:
- Around line 142-156: The TypedTableElement handling in the table parsing flow
is dropping index tablespace information for table constraints. Update the
TypedTableElement loop in table.rs so that, like the sibling TableElement path,
it captures OptConsTableSpace from parsed TableConstraint values and assigns it
to table.index_tablespace before calling apply_constraint. Ensure PRIMARY
KEY/UNIQUE constraints with USING INDEX TABLESPACE round-trip correctly through
table_constraint and apply_constraint.
---
Nitpick comments:
In `@src/pull/mod.rs`:
- Around line 1098-1129: Both apply_column_comment and apply_trigger_comment
duplicate the same target.schema split-and-table lookup logic, so extract that
resolution into a shared helper in this module (for example,
resolve_owning_table or similar) and have both comment handlers call it before
updating the column or trigger. Keep the helper responsible for splitting the
relation into schema/table and calling find_table on a QualifiedName so the two
handlers only contain their specific comment assignment logic.
In `@src/pull/writer.rs`:
- Around line 51-85: The serialization logic for views and materialized views is
duplicated in the writer flow, so extract the shared behavior into a small
helper or closure around serialize, view_dependencies, and writer.save_value.
Reuse that helper for both assembly.views and assembly.materialized_views,
passing only the plural key ("views" or "materialized_views") and preserving the
dependency injection into the serialized value. Make sure the helper works
cleanly with writer and the borrow checker.
🪄 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 Plus
Run ID: fe25e4a4-59ea-4f3b-95d6-ff09cc167f0f
📒 Files selected for processing (10)
fixtures/schema.sqlschemata/table.ymlsrc/build/mod.rssrc/ddl/mod.rssrc/ddl/table.rssrc/deploy/mod.rssrc/pull/mod.rssrc/pull/writer.rstests/build_parity.rstests/deploy.rs
The TypedTableElement loop applied table constraints via apply_constraint but never captured OptConsTableSpace into table.index_tablespace, unlike the sibling TableElement loop. A typed table's PRIMARY KEY/UNIQUE ... USING INDEX TABLESPACE constraint silently lost its tablespace on pull — the same round-trip data loss class this branch fixes elsewhere. Mirror the sibling loop: first constraint carrying a tablespace wins. Reported by CodeRabbit. just check and just gates pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Owner
Author
|
🤖 This comment was posted by Claude on behalf of @gmr Addressed CodeRabbit's review (1 comment):
|
Owner
Author
|
Automated monitoring complete — PR is ready to merge.
No further blockers. The documented intentional design decisions (schema-qualified functions per build deviation #12, split SERIAL/sequence Co-Authored-By: Claude Opus 4.8 (1M context) noreply@anthropic.com |
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.
Batch 4 of the review-findings work. Fixes the six pull/build bugs that batch 3's fixture work uncovered, then re-enables the deferred fixtures so each fix is locked in by the round-trip and deploy gates. Expanding the fixtures surfaced three further genuine bugs, also fixed here.
The six batch-4 bugs
CREATE TABLEplus a separateTABLE ATTACH(ALTER TABLE ONLY parent ATTACH PARTITION child FOR VALUES ...); the parser only handled inlinePARTITION OF, and the ingest had no arm forTableAttach, so partition membership was lost. Now parsed and folded into the parent.SET DEFAULTdropped on pull — pg_dump emits column defaults (e.g.nextval(...)) as a separateALTER TABLE ... ALTER COLUMN ... SET DEFAULT; pull now folds them back onto the column.apply_commentgained a TRIGGER arm.CREATE FUNCTION namewith no().Further bugs surfaced by expanding the fixtures
for_values_totypo in the partition schema — the RANGEoneOfbranch required a misspelled key, so no range-partitioned table could validate.CREATE FUNCTION name(...)failspg_restoreunder its resetsearch_path; now schema-qualified (matches pg_dump; documented as build deviation Phase 3: ddl module — object, view, function, and trigger families #12).CREATE TABLE— created an unrestorable dependency cycle (sequenceOWNED BYthe column); now split into a separateALTER TABLE ... SET DEFAULTentry depending on the sequence, as pg_dump does.COMMENT ON TRIGGER/INDEXresolved to no owning item and was silently skipped on fresh deploy; deploy now resolves owners transitively through intermediate non-item entries.Validation
just check— fmt, clippy-D warnings, and all unit + integration tests pass (including new unit tests for the ATTACH fold, the sequence-default split, andnextvaltarget parsing, plus updatedbuild_paritydeviations).just gates— round-trip and all three deploy gates (equivalence, convergence, safety) pass against Postgres 17 with the expanded fixtures.🤖 Generated with Claude Code
Summary by CodeRabbit
ALTER TABLE ... SET DEFAULTandATTACH PARTITIONfolding into parent tables.DEFAULTentries.tobounds.