feat(ogar-adapter-surrealql): AST -> Class walk (closes ADR-017)#32
Conversation
The `parse_surrealql_ddl` function now actually walks the SurrealDB AST and lifts the supported DDL subset into `Vec<Class>`. PR #24 wired the parser for syntax validation only; this PR completes the walk for the v1 subset. # Supported in this walk - DEFINE TABLE <name> -> Class - DEFINE FIELD <field> ON <table> TYPE string | int | bool | datetime | float | decimal | uuid | bytes | object | duration | any -> Attribute - DEFINE FIELD <field> ON <table> TYPE record<X> -> Association(BelongsTo) - DEFINE FIELD <field> ON <table> TYPE option<record<X>> -> Association(BelongsTo, optional=true) - DEFINE FIELD <field> ON <table> TYPE option<<primitive>> -> Attribute(type=option<primitive>) - Implicit-table-from-field (no preceding DEFINE TABLE) — the walker creates the class lazily on first field encounter. - Multi-table definition order preserved. # Not yet supported (intentional next-sprint shape) - DEFINE FIELD … ASSERT $value IN [...] (EnumDecl::Static lift) - VALUE / DEFAULT / COMPUTED clauses - PERMISSIONS / FLEXIBLE / READONLY - DEFINE EVENT (lifecycle -> ActionDef) - DEFINE INDEX (not part of OGAR IR; ignored) - Comment-marker recovery for non-owning sides (`-- {table} HasMany {name}` is a SurrealQL comment, invisible to the parser; the non-owning side is recoverable from the owning side's record<X> in a separate post-pass over the full Vec<Class>). # AST walker architecture New private `walk` module under `#[cfg(feature = "surrealdb-parser")]`: - `walk_query(&Ast, Query) -> Vec<Class>` — entry point. - `visit_define(&Ast, NodeId<Expr>, &mut by_name, &mut order)` matches on Expr::DefineTable / Expr::DefineField. - `expr_to_simple_name(&Ast, NodeId<Expr>) -> Option<String>` extracts identifier names from `Expr::Path { start: Ident, parts: None }`. - `lift_field_type(&Ast, Option<NodeId<Type>>) -> FieldShape` classifies the Type AST into Primitive | Record | Untyped | Other. Handles the `option<X>` encoding (NodeList<PrimeType> prefixed with PrimeType::None — per surrealdb/parser/src/ parse/kind.rs at T![OPTION]). - `iter_node_list<T>(&Ast, NodeListId<T>)` — closure-based iterator over the AST's linked-list structure. # Round-trip tests 8 new tests on top of the 14 pre-existing. The shape: walk_minimal_table_produces_class walk_table_with_string_attribute walk_table_with_belongs_to_record_field walk_table_with_optional_belongs_to walk_multi_table_preserves_definition_order walk_round_trip_simple_class <- emit then parse, structural eq walk_round_trip_belongs_to_association walk_implicit_table_from_field_alone walk_returns_parse_error_on_invalid_ddl # Producer of `schema_ddl_hint` (the loop closure) PR #25 introduced `KnowableFromStore::register(class_identity, schema_ddl_hint: Option<&str>)` — the registry's "self-describing" clause. Today every caller passes `None`. This PR makes `emit_surrealql_ddl(&[class.clone()])` the canonical producer of the hint, closing the loop that PR #25's docstring named ("Future PRs can render via ogar-adapter-surrealql::emit_surrealql_ddl"). A follow-up can wire that call into `register_class_knowable_from`'s body. # CI floor extended `.github/workflows/ci.yml` adds: cargo test -p ogar-adapter-surrealql --features surrealdb-parser The default `cargo test --workspace` step doesn't enable the feature (the parser dep is heavy and not needed by other workspace members); this extra step makes the walker tests part of the CI floor. ~30-60s added to CI; bounded scope (one crate). # Verification Local: - `cargo test -p ogar-adapter-surrealql --features surrealdb-parser` -> 22 passed (14 prior + 8 walker tests). - `cargo test --workspace` (default, no feature) -> clean (the walker code is behind cfg; the existing parser-off test still runs). - `cargo check --workspace --all-targets` -> clean. Closes ADR-017 (the surrealql-AST-to-Class walk) for the v1 subset. https://claude.ai/code/session_01PBTGaPCSnnt6u3pjXpbLwY
![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: 8699ee027d
ℹ️ 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".
| fn primitive(name: &str, optional: bool) -> FieldShape { | ||
| if optional { | ||
| FieldShape::Primitive(format!("option<{name}>")) |
There was a problem hiding this comment.
When the input contains a supported optional primitive such as DEFINE FIELD email ON account TYPE option<string>;, this stores Attribute.type_name = "option<string>", but emit_field_attr later sends type names through map_type_to_surrealql, which has no option<...> arm and re-emits it as an unmapped string type. This breaks the advertised parse/emit roundtrip for the newly supported option<<primitive>> subset and also misses the existing AttributeOptions.required = Some(false) representation.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 6bf88d9.
The walker now stores Attribute.type_name = "string" (bare) + attr.options.required = Some(false) — exactly the IR-canonical shape ogar-vocab::AttributeOptions documents. emit_field_attr reads options.required == Some(false) and wraps with SurrealQL option<…> on egress.
Verifying tests:
walk_table_with_optional_primitive_uses_required_false(line 998): parsesTYPE option<string>and assertstype_name == "string"+options.required == Some(false)— explicitly rejects the "option" baked-into-type_name shape.walk_round_trip_optional_primitive(line 1014): builds the IR shape → emits → asserts the DDL containsTYPE option<string>;(not the unmapped/* … */comment) → re-parses → asserts the IR matches.
Round-trip preserved across both directions.
| let target = ident_list | ||
| .idents | ||
| .and_then(|ids| iter_node_list(ast, ids).next()) | ||
| .map(|id| { |
There was a problem hiding this comment.
For SurrealQL fields like TYPE record<user | administrator>, the AST carries multiple identifiers, but this walker takes only .next() and records a BelongsTo to the first table. That silently changes the schema when re-emitted to record<user> and loses valid target tables; if OGAR cannot represent a union association yet, this should fall back to Other/unmappable rather than narrowing the constraint.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 6bf88d9.
In lift_field_type's PrimeType::Record arm, the walker now peeks for a second ident; if present, returns FieldShape::Other (typeless "any" Attribute placeholder) instead of silently narrowing to record<user>.
Concretely:
let first = match iter.next() { Some(id) => id, None => return FieldShape::Other };
if iter.next().is_some() {
return FieldShape::Other; // multi-target — preserve faithfully or reject
}OGAR's Association::class_name: Option<String> can't represent a union — the honest fallback keeps round-trip from mutating the schema. Future PR can grow Vec<String> targets or AssociationKind::Union { targets } if a real consumer needs it.
Verifying test:
walk_multi_target_record_returns_other_not_first_target(line 1047): assertsassociations.is_empty()(no silentBelongsTo→user) and only the "any" placeholder Attribute lands.
… multi-target reject Two correctness bugs Codex flagged on PR #32 — both real, both with verifying round-trip tests. # P2 #1 — optional primitives now round-trip The walker stored `Attribute.type_name = "option<string>"` for `TYPE option<string>;`. The emitter's `map_type_to_surrealql` has no `option<...>` arm, so it fell through to `format!("string /* unmapped producer type: {other} */")` — breaking the round-trip claim for the newly supported option<<primitive>> subset and missing the existing `AttributeOptions.required = Some(false)` representation. Fix (both directions): Walker: FieldShape::Primitive(String) → FieldShape::Primitive { type_name: String, optional: bool } For optional primitives, stores BARE type + sets `attr.options.required = Some(false)`. The IR-canonical shape per `ogar-vocab::AttributeOptions`. Emit: emit_field_attr reads `attr.options.required == Some(false)` and wraps the SurrealQL type with `option<…>`. `None` remains "unset" (not "required=true") — leave unwrapped. Tests added (verifying): - `walk_table_with_optional_primitive_uses_required_false` — parses `TYPE option<string>`, asserts bare type + required=Some(false). - `walk_round_trip_optional_primitive` — builds the IR shape, emits, re-parses, asserts the same shape comes back. CRITICAL: asserts the emitted DDL contains `TYPE option<string>;`, NOT a `/* unmapped … */` comment. # P2 #2 — multi-target record<a | b> rejected, not narrowed The walker took `iter_node_list(ids).next()` on the record target list and recorded a BelongsTo to the first table only. For `TYPE record<user | administrator>`, the walker silently lost `administrator` — re-emitting as `record<user>` mutates the schema. Fix: in `lift_field_type`'s `PrimeType::Record` arm, peek for a second ident; if present, return `FieldShape::Other` (the typeless "any" Attribute placeholder). OGAR's `Association::class_name: Option<String>` can't represent a union; the honest fallback keeps round-trip from mutating the schema. Future PR can grow union-target support if a real consumer needs it (the change shape would be `Association::class_name: Vec<String>` or a new `AssociationKind::Union { targets: Vec<String> }` variant). Test added (verifying): - `walk_multi_target_record_returns_other_not_first_target` — asserts associations is EMPTY (no silent BelongsTo→user) and only a typeless "any" Attribute is recorded. # Verification cargo test -p ogar-adapter-surrealql --features surrealdb-parser → 25 passed (22 prior + 3 new). cargo check --workspace --all-targets → clean cargo test --workspace → clean PII abort-guard scan: CLEAN. Closes Codex P2 #1 + #2 on PR #32. https://claude.ai/code/session_01PBTGaPCSnnt6u3pjXpbLwY
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6bf88d905e
ℹ️ 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".
| PrimeType::Bool(_) => primitive("bool", optional), | ||
| PrimeType::DateTime(_) => primitive("datetime", optional), | ||
| PrimeType::Float(_) => primitive("float", optional), | ||
| PrimeType::Decimal(_) | PrimeType::Number(_) => primitive("decimal", optional), |
There was a problem hiding this comment.
When parsing an existing DEFINE FIELD ... TYPE number, this branch stores the field as OGAR type decimal; the emitter then renders that back as TYPE decimal because map_type_to_surrealql has no number case. That makes a parse/emit round-trip narrow a SurrealQL number field to decimal, mutating schemas that intentionally allow the generic numeric type.
Useful? React with 👍 / 👎.
…+-only; extract() is todo!()) — mirrors ruff #32 Codex fixes
2 participants
Summary
parse_surrealql_ddlnow actually walks the SurrealDB AST and lifts the supported DDL subset intoVec<Class>. Closes ADR-017 for the v1 subset (the prioritized substantive item per the other session's queue guidance: "surrealql walk first; defer adapter-ttl until #25'sschema_ddl_hinthas at least one real producer").Supported in this walk
DEFINE TABLE <name>Class { name }DEFINE FIELD <f> ON <t> TYPE string|int|bool|datetime|float|decimal|uuid|bytes|object|duration|anyAttribute { name, type_name: "<surrealql-canonical>" }DEFINE FIELD <f> ON <t> TYPE record<X>Association { kind: BelongsTo, class_name: "X" }DEFINE FIELD <f> ON <t> TYPE option<record<X>>Association { kind: BelongsTo, optional: true }DEFINE FIELD <f> ON <t> TYPE option<<primitive>>Attribute { type_name: "option<<primitive>>" }DEFINE FIELD <f> ON <t>(no TYPE)AttributeNot yet supported (intentional, next-sprint shape)
ASSERT $value IN [...]→EnumDecl::Static(requires walking BinaryExpr tree forIN [list]pattern)VALUE,DEFAULT,COMPUTED,PERMISSIONS,FLEXIBLE,READONLYDEFINE EVENT→ActionDef(lifecycle lift)DEFINE INDEX(not part of OGAR IR; ignored)-- {table} HasMany {name}is a SurrealQL comment, invisible to the parser. Non-owning side is recoverable from the owning side'srecord<X>in a post-pass overVec<Class>(separate PR).Architecture — new private
walkmoduleBehind
#[cfg(feature = "surrealdb-parser")]:Handles the
option<X>encoding correctly — the parser represents it asNodeList<PrimeType>prefixed withPrimeType::None(persurrealdb/parser/src/parse/kind.rsat theT![OPTION]arm). The walker detects the leadingNoneand setsoptional=trueon the recovered association / wraps the primitive type name.Round-trip tests (8 new)
Total in
ogar-adapter-surrealql: 22 tests passing (14 emit-only prior + 8 walker).Closes the
schema_ddl_hintloopPR #25's
KnowableFromStore::register(class_identity, schema_ddl_hint: Option<&str>)had a TODO in the docstring: "Future PRs can render viaogar-adapter-surrealql::emit_surrealql_ddl(&[class.clone()])." This PR makes that emit function the canonical producer of the hint. A small follow-up can wireemit_surrealql_ddl(&[class.clone()])intoregister_class_knowable_from's body, finally making the "self-describing registry" claim concrete (rather than aspirational).CI floor extended
.github/workflows/ci.ymladds one step:The default
cargo test --workspacedoesn't enable the surrealdb-parser feature (the surrealdb fork's deps are heavy and not needed by other workspace members); this extra step covers the walker. Adds ~30-60s to CI, bounded scope (one crate).Verification
PII abort-guard scan on both changed files: CLEAN (word-boundary matching).
Position in the sequencing
Per
docs/RDF-OWL-ALIGNMENT.md §10:ogar-adapter-ttlscaffold — deferred per the other session's recommendation ("not until feat: ogar-knowable-from — trait-mediated §10.3 producer seam (ADR-010 OGAR-side close) #25 has a real producer", which this PR now provides)This PR closes ADR-017 (the original substantive item) AND unlocks Phase 2 (TTL adapter now has a real
schema_ddl_hintproducer to compose against).https://claude.ai/code/session_01PBTGaPCSnnt6u3pjXpbLwY