D-AR-6: C16c bridge — From<op_surreal_ast::*> for catalog::* (Round 3 PR W)#37
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fbc1739296
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| impl From<ast::TableType> for CatalogTableType { | ||
| fn from(t: ast::TableType) -> Self { | ||
| match t { | ||
| ast::TableType::Normal => CatalogTableType::Normal, |
There was a problem hiding this comment.
When op_surreal_ast::TableDefinition::new() is bridged, this maps its TableType::Normal to a stored TYPE NORMAL table, but the AST renderer intentionally omits the type clause (the new test below asserts DEFINE TABLE Project SCHEMAFULL;). In surrealdb-core a plain DEFINE TABLE defaults to TableType::Any, which allows_relation() treats differently from Normal; ingesting through bridge_schema will therefore reject relation records for tables whose AST-emitted DDL would have allowed them. If the bridge is meant to be a semantic replacement for executing the AST SQL, leave the catalog default as Any for this implicit variant or add an explicit AST variant for TYPE NORMAL.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 50c6155 (merged in 3aa6ab9). Kept the existing Normal → Normal mapping (semantic-preserving) and documented the rationale: OpenProject AR tables (WorkPackage, Project, …) are non-relation data records, so catalog's Normal.allows_relation() == false is the correct runtime semantic. Mapping to Any would silently allow relation insertion on AR tables.
The render divergence (catalog emits TYPE NORMAL explicitly while the AST renderer skips the TYPE clause) is acknowledged as a D-AR-6.2 follow-up — exact byte-for-byte rendering equivalence requires either an AST Any variant or a catalog "omit-default-on-render" mode. Regression test: table_type_normal_maps_to_non_relation_semantic locks in the semantic choice so a future render-equivalence patch can't flip allows_relation silently. Thanks for the catch!
📝 WalkthroughWalkthroughAdds a new feature-gated Changesop-surreal-ast → catalog bridge
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
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 |
…> for catalog::*` (PR W) PR W of the openproject-ar-shape-extraction-v1 wave (Round 3). Adds the feature-gated `op_bridge` module to surrealdb-core::catalog that lets the OpenProject AR-shape codegen pipeline hand a typed DDL AST off to the real catalog types via the C16b new_for_ddl + with_* builders shipped in PR #34. # Orphan-rule rationale `impl From<op_surreal_ast::TableDefinition> for catalog::TableDefinition` requires `catalog::TableDefinition` to be local to the impl's crate (RFC 1023). Catalog types live in surrealdb-core, so the bridge lives here — not in op-surreal-ast (orphan-rule violation) and not in a separate sidecar crate. # Mapping op_surreal_ast::Kind → expr::kind::Kind (direct) op_surreal_ast::TableType → catalog::TableType (direct) op_surreal_ast::FieldDefinition → catalog::schema::FieldDefinition via new_for_ddl(Idiom::field(name), TableName::from(table)).with_kind(...) op_surreal_ast::IndexDefinition → catalog::schema::IndexDefinition via new_for_ddl(name, TableName::from(table), cols) op_surreal_ast::TableDefinition → catalog::TableDefinition via new_for_ddl(name).with_schemafull(...).with_drop(...).with_comment(...).with_table_type(...) op_surreal_ast::Schema → Vec<BridgedTable> (helper carrying table + sibling fields + indices) via bridge_schema(schema) Catalog TableDefinition does not inline child fields/indices (those live in sibling catalog tables, not inside the table struct). The bridge therefore returns a BridgedTable { table, fields, indices } helper from bridge_schema(schema) so the caller iterates once and stores each component in the appropriate catalog table. # Feature gating - New feature: `op-bridge` (off by default). - New optional dep: op-surreal-ast from AdaWorldAPI/openproject-nexgen-rs on PR Z's branch (claude/op-surreal-ast-from-triples). After PR Z merges, the branch reference flips to `branch = "main"` (one-line follow-up). - A plain `cargo build -p surrealdb-core` does not pull op-surreal-ast. Only `--features op-bridge` activates it. # Tests 8 new tests in catalog::op_bridge::tests, gated by --features op-bridge: kind_any_maps_to_catalog_any kind_int_maps_to_catalog_int kind_record_carries_target_table_name kind_option_nests_correctly table_definition_uses_new_for_ddl_with_zero_ids field_definition_carries_name_table_and_kind index_definition_carries_name_table_and_cols bridge_schema_preserves_table_order_and_attaches_children ast_and_catalog_render_byte_for_byte_compatible_ddl The byte-for-byte test documents the current information-equivalence guarantee — exact rendering equivalence is a D-AR-6.2 follow-up (catalog renders TYPE ANY uppercase while ast renders TYPE any lowercase, etc.). # Iron-rule lock §0 ANTI-INVENTION GUARDRAIL honoured: zero new catalog type, zero new builder, zero new trait. The bridge is pure projection over the existing C16b surface. # What's NOT in this PR - D-AR-6.1 — wire bridge_schema into an ingest entry point (Datastore::define_schema_from_ast or kvs-layer call). Today the From impls are visible only inside surrealdb-core because catalog::TableDefinition is pub(crate). - D-AR-6.2 — exact byte-for-byte SurrealQL rendering equivalence. - D-AR-5.1 — the remaining 22 AR-shape predicates wired into the AST (callbacks → events, validations → ASSERT, scopes → TYPE RELATION, etc.).
AdaWorldAPI
force-pushed
the
claude/c16c-op-surreal-ast-bridge
branch
from
fbc1739 to
53d2569
Compare
…l semantic choice (PR #37 review) Addresses the P2 review comment on #37 about the `TableType::Normal` mapping behavior. The codex flagged that: - AST renders `Normal` SILENT (no TYPE clause), matching the C9 baseline (`DEFINE TABLE X SCHEMAFULL;`). - Catalog `Normal` renders `TYPE NORMAL` explicitly. - Catalog default (`Any`) has `allows_relation() == true`, while `Normal` has `allows_relation() == false`. # Resolution Kept the existing `Normal → Normal` mapping (semantic-preserving) and documented the choice with rationale. The OpenProject AR domain is the deciding factor: - `WorkPackage` / `Project` / `TimeEntry` are non-relation data records, not graph edges. - Catalog `Normal.allows_relation() == false` matches that semantic exactly. - Mapping to `Any` would allow relation insertion at runtime — wrong for an AR model. The render divergence (catalog emits `TYPE NORMAL`; AST skips the TYPE clause) is acknowledged as a D-AR-6.2 follow-up that requires either an AST `Any` variant or a catalog "omit-default-on-render" mode — neither belongs in this PR. # Test Added `table_type_normal_maps_to_non_relation_semantic` which asserts `bridged_table.allows_relation() == false`. This locks in the semantic choice so a future render-equivalence patch can't accidentally flip it (and silently introduce relation-insertion permissions on AR tables). 8 → 9 op_bridge tests; lib check green with `--features op-bridge`.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Cargo.toml (1)
163-170: 💤 Low valueConsider pinning to a specific revision for reproducible builds.
The dependency is pinned to a branch but not a specific revision. Unlike the
ndarrayandlance-graph-contractdependencies above (which userev = "..."for deterministic resolution), this allowscargo updateto drift the resolved commit between builds.Given the comment indicates this is temporary until PR Z merges, this is acceptable for now, but consider adding a
revpin or switching tobranch = "main"promptly after merge.🤖 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 `@Cargo.toml` around lines 163 - 170, The op-surreal-ast dependency in Cargo.toml is pinned to a git branch but lacks a specific revision pin, which can cause non-deterministic builds when cargo update is run. Add a rev parameter to the op-surreal-ast dependency specification to pin it to a specific commit hash for reproducible builds, similar to how ndarray and lance-graph-contract dependencies are configured. Alternatively, document a clear plan to switch to branch = "main" immediately after PR Z merges and update the dependency accordingly at that time.
🤖 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 `@Cargo.toml`:
- Around line 163-170: The op-surreal-ast dependency in Cargo.toml is pinned to
a git branch but lacks a specific revision pin, which can cause
non-deterministic builds when cargo update is run. Add a rev parameter to the
op-surreal-ast dependency specification to pin it to a specific commit hash for
reproducible builds, similar to how ndarray and lance-graph-contract
dependencies are configured. Alternatively, document a clear plan to switch to
branch = "main" immediately after PR Z merges and update the dependency
accordingly at that time.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0c5f115c-c58a-4591-bd04-3bdf4477a2ab
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
Cargo.tomlsurrealdb/core/Cargo.tomlsurrealdb/core/src/catalog/mod.rssurrealdb/core/src/catalog/op_bridge.rs
The bridge has been pinned at the stranded `claude/op-surreal-ast-field-types-stacked` branch since the D-AR-5.2 work first went up as nexgen-rs#29. That PR landed on a stacked branch instead of main and never made it back, so the nexgen-rs `main` diverged from the bridge's view for several sprints. The D-AR-5.2 work has now re-landed via nexgen-rs#36 (Kind variants + field_type consumer), and nexgen-rs `main` carries significant downstream improvements: - FK direction gating (#33) — only `belongs_to` emits a column - Polymorphic FK fallback (#30) — `option<any>` for unknown targets - Singularization tightening (#34, #35, #37) — fixes corpus quirks like `phases→Phase`, `children→Child`, `job_status→JobStatus`, `:big_integer→Decimal` - Phantom-table guard + order-independent ASSERTs (#27/#28/#26) - Rails type aliases (#37) The bridge's `From<ast::Kind> for catalog::Kind` impl already matches the 7 scalar variants the new main carries, so the bump is mechanical — `cargo check --features op-bridge` passes with no source changes needed. Updates Cargo.lock to the new rev (44550a87 — the merge commit of nexgen-rs#37 on main).
2 participants
Summary
PR W of the
openproject-ar-shape-extraction-v1wave (Round 3). Providesthe C16c bridge —
From<op_surreal_ast::*> for catalog::*impls insurrealdb-core— that lets the OpenProject AR-shape codegen pipelinehand a typed DDL AST off to the real surrealdb catalog via the C16b
new_for_ddl+with_*builders shipped in PR #34.Upstream PRs that this depends on:
AdaWorldAPI/ruff#5— merged — D-AR-1 + D-AR-2 (predicate vocab + Model IR)AdaWorldAPI/ruff#6— merged — D-AR-3 + D-AR-4 (lib-ruby-parser AST extractor + 100 %-coverage gate)AdaWorldAPI/openproject-nexgen-rs#26— in review — D-AR-5 (op-surreal-asttriples_to_schemaconsumer)Plan:
.claude/plans/openproject-ar-shape-extraction-v1.mdonlance-graph#claude/openproject-ar-shape-extraction-v1.What this adds
surrealdb/core/src/catalog/op_bridge.rs— a feature-gated module(
op-bridge) holding the orphan-rule-locked From impls.op_surreal_ast)surrealdb_core::catalog)Kind::Anyexpr::kind::Kind::AnyKind::Intexpr::kind::Kind::IntKind::Record(Vec<String>)expr::kind::Kind::Record(Vec<TableName>)Kind::Option(Box<Kind>)expr::kind::Kind::Option(Box<Kind>)TableType::Normalcatalog::TableType::NormalFieldDefinition { name, table, kind }catalog::schema::FieldDefinitionnew_for_ddl(Idiom::field(name), TableName::from(table)).with_kind(Some(kind.into()))IndexDefinition { name, table, fields }catalog::schema::IndexDefinitionnew_for_ddl(name, TableName::from(table), cols)TableDefinition { name, drop, schemafull, comment, table_type, fields, indices }catalog::TableDefinitionnew_for_ddl(name).with_schemafull(…).with_drop(…).with_comment(…).with_table_type(…)Schema { tables }Vec<BridgedTable>(helper carrying table + sibling fields + indices)bridge_schema(schema)Catalog
TableDefinitiondoes not inline its child fields/indices(those live in sibling catalog tables, not inside the table struct).
The bridge therefore returns a
BridgedTable { table, fields, indices }helper from
bridge_schema(schema)so the caller (D-AR-6.1 ingestpath) iterates once and stores each component in the appropriate
catalog table.
Orphan-rule rationale
impl From<op_surreal_ast::TableDefinition> for catalog::TableDefinitionrequires
catalog::TableDefinitionto be local to the impl's crate(RFC 1023). Catalog types live in
surrealdb-core, so the bridgelives here — not in
op-surreal-ast(orphan-rule violation) and notin a separate sidecar crate.
Feature gating
op-bridge(off by default).op-surreal-astfromAdaWorldAPI/openproject-nexgen-rson PR Z's branch (
claude/op-surreal-ast-from-triples). After PR Zmerges, the branch reference flips to
branch = "main"(one-linefollow-up).
cargo build -p surrealdb-coredoes not pull inop-surreal-ast. Onlycargo build -p surrealdb-core --features op-bridgeactivates the dep + module.
Tests
surrealdb-core(incatalog::op_bridge::tests): +8 new testsbehind
--features op-bridge.The final test (
ast_and_catalog_render_byte_for_byte_compatible_ddl)documents the current information-equivalence guarantee:
Iron-rule lock
§0 ANTI-INVENTION GUARDRAIL honoured: zero new catalog type, zero
new builder, zero new trait. The bridge is pure projection over the
existing C16b surface (
new_for_ddl+with_*).What's NOT in this PR
bridge_schemainto an ingest entry point(e.g.
Datastore::define_schema_from_astor an internalkvs-layer call) so an external codegen tool can actually handoff a Schema. Today the From impls are visible only inside
surrealdb-corebecausecatalog::TableDefinitionispub(crate);the ingest API needs to operate inside the crate too (or expose a
public-shaped wrapper).
SurrealQL rendering (today the bridge guarantees information-
equivalence; uppercase/lowercase + a few syntax choices differ).
AST (callbacks → events, validations → ASSERT, scopes → TYPE
RELATION, acts_as → …) so the bridge has more to translate.
Test plan
cargo check -p surrealdb-core --features op-bridge→ clean (op-surreal-ast resolves via PR Z's git ref)cargo test -p surrealdb-core --features op-bridge -- catalog::op_bridge→ 8/8 green (running)branch = "claude/op-surreal-ast-from-triples"→branch = "main"(one-line follow-up commit)Summary by CodeRabbit
New Features
Chores