From dab0af042aed6dc6f09a46019ecc1e71bd93c615 Mon Sep 17 00:00:00 2001 From: "Claude (OGAR session)" Date: Fri, 5 Jun 2026 11:46:21 +0000 Subject: [PATCH] fix(ogar-adapter-clickhouse-ddl): preserve backtick-quoted dotted table names on parse MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes Codex P2 on PR #38 (now merged). # The bug When the emitter produces `CREATE TABLE `sale.order` (…)` for an Odoo dotted class name, the parser path went: ct.name (ObjectName) ──to_string──▶ "`sale.order`" ──last_ident──▶ rsplit('.').next() = "order`" ──trim_matches('`') = "order" ^^^^^^ WRONG The `.` was treated as a database/table separator even though it was INSIDE the quoted identifier. Result: emit + parse round-trip turned `sale.order` into `order`, breaking the round-trip the PR claimed to support for Odoo dotted names. # The fix Bypass the string round-trip entirely. sqlparser already resolves the backtick / dot ambiguity structurally: - `\`sale.order\`` → `ObjectName(vec![ObjectNamePart::Identifier( Ident { value: "sale.order", quote_style: Some('`') })])` — ONE part because the backticks group it. - `db.table` → `ObjectName(vec![db_part, table_part])` — TWO parts. The new helper accesses the last part's `Ident.value` directly (the *unquoted* text), which is correct for both cases without splitting: fn last_ident_from_object_name(name: &ObjectName) -> String { name.0.last() .and_then(|part| part.as_ident()) .map(|ident| ident.value.clone()) .unwrap_or_default() } # Verifying tests (4 new; 12 prior → 16 total) parse_backtick_quoted_dotted_table_keeps_full_name — `CREATE TABLE \`sale.order\` (…)` → name == "sale.order" (not "order" — the Codex P2 motivating case). parse_qualified_db_table_takes_last_segment — `CREATE TABLE my_db.users (…)` → name == "users" (qualified-db path still works). round_trip_odoo_dotted_table_name — build IR with Class::new("sale.order") + attribute, emit, parse, assert name + attribute survive byte-identical. round_trip_three_segment_odoo_name — `account.move.line` three-segment name round-trips. # Verification cargo test -p ogar-adapter-clickhouse-ddl -> clean cargo test -p ogar-adapter-clickhouse-ddl --features clickhouse-parser -> 16/16 cargo check --workspace --all-targets -> clean PII abort-guard (word-boundary): CLEAN. # Symmetry note This is the same class of bug ogar-adapter-surrealql shipped a fix for in PR #36 (backtick-quoting on emit + symmetric parser handling). The two adapters now follow the same discipline: - surrealql adapter: emit backtick-quotes non-bare idents; walker accesses Path.start.text directly (unquoted) — no string round-trip on the parse side. - clickhouse adapter: emit backtick-quotes non-bare idents; walker accesses ObjectNamePart.Ident.value directly (unquoted) — no string round-trip on the parse side. Same lesson: don't string-round-trip when the AST already encodes the structural distinction. https://claude.ai/code/session_01PBTGaPCSnnt6u3pjXpbLwY --- crates/ogar-adapter-clickhouse-ddl/src/lib.rs | 105 +++++++++++++++++- 1 file changed, 99 insertions(+), 6 deletions(-) diff --git a/crates/ogar-adapter-clickhouse-ddl/src/lib.rs b/crates/ogar-adapter-clickhouse-ddl/src/lib.rs index 8787a51..1e62a7f 100644 --- a/crates/ogar-adapter-clickhouse-ddl/src/lib.rs +++ b/crates/ogar-adapter-clickhouse-ddl/src/lib.rs @@ -242,7 +242,7 @@ fn quote_ch_ident(name: &str) -> String { mod walk { use super::*; use ogar_vocab::Attribute; - use sqlparser::ast::{ColumnDef, DataType, Statement}; + use sqlparser::ast::{ColumnDef, DataType, ObjectName, Statement}; use sqlparser::dialect::ClickHouseDialect; use sqlparser::parser::Parser; @@ -253,7 +253,7 @@ mod walk { let mut classes = Vec::new(); for stmt in stmts { if let Statement::CreateTable(ct) = stmt { - let name = last_ident(&ct.name.to_string()); + let name = last_ident_from_object_name(&ct.name); let mut class = Class::new(name); for col in &ct.columns { class.attributes.push(lift_column(col)); @@ -359,10 +359,31 @@ mod walk { ) } - /// Get the last `.`-separated segment of an identifier — handles - /// fully qualified names like `database.table`. - fn last_ident(s: &str) -> String { - s.rsplit('.').next().unwrap_or(s).trim_matches('`').to_string() + /// Get the table name from an `ObjectName` — handles both: + /// 1. **Multi-part qualified names** like `database.table` — + /// sqlparser surfaces this as `ObjectName(vec![db_part, + /// table_part])`; the table name is the last segment. + /// 2. **Backtick-quoted dotted single names** like + /// `` `sale.order` `` (Odoo convention) — sqlparser surfaces + /// this as `ObjectName(vec![one_part_with_value="sale.order"])`; + /// the dot is INSIDE the identifier value, not a separator. + /// + /// Closes Codex P2 on PR #38: the previous `last_ident(&str)` + /// converted the `ObjectName` to a string and then split on `.` + /// before stripping backticks, which silently turned + /// `` `sale.order` `` into `order` and broke the round-trip the + /// PR claimed to support for Odoo dotted class names. + /// + /// Using `Ident.value` directly bypasses the string round-trip + /// — `value` is the *unquoted* text, so backtick-quoting and the + /// `.` ambiguity are resolved structurally by the parser before + /// this helper sees it. + fn last_ident_from_object_name(name: &ObjectName) -> String { + name.0 + .last() + .and_then(|part| part.as_ident()) + .map(|ident| ident.value.clone()) + .unwrap_or_default() } } @@ -505,6 +526,78 @@ mod tests { } } + // ── Codex P2 fix (PR #38) ─────────────────────────────────────────── + // Backtick-quoted dotted table names (Odoo `sale.order` convention) + // round-trip correctly. Previously `last_ident` split on `.` BEFORE + // stripping backticks, silently truncating `` `sale.order` `` to + // `order` and breaking the round-trip the PR claimed to support. + // ──────────────────────────────────────────────────────────────────── + + #[cfg(feature = "clickhouse-parser")] + #[test] + fn parse_backtick_quoted_dotted_table_keeps_full_name() { + let ddl = "CREATE TABLE `sale.order` (id UInt64) ENGINE = MergeTree ORDER BY id;"; + let classes = parse_clickhouse_ddl(ddl).expect("parse OK"); + assert_eq!(classes.len(), 1); + // CRITICAL: must be "sale.order", not "order" (the Codex P2 + // motivating case). + assert_eq!(classes[0].name, "sale.order"); + } + + #[cfg(feature = "clickhouse-parser")] + #[test] + fn parse_qualified_db_table_takes_last_segment() { + // Multi-part qualified name `db.table` (unquoted) — sqlparser + // surfaces as ObjectName with two parts; the table name is the + // last segment. + let ddl = "CREATE TABLE my_db.users (id UInt64) ENGINE = MergeTree ORDER BY id;"; + let classes = parse_clickhouse_ddl(ddl).expect("parse OK"); + assert_eq!(classes.len(), 1); + assert_eq!(classes[0].name, "users"); + } + + #[cfg(feature = "clickhouse-parser")] + #[test] + fn round_trip_odoo_dotted_table_name() { + // The load-bearing round-trip: build IR with an Odoo dotted + // name, emit, parse, assert the name survives byte-identical. + let mut original = Class::new("sale.order"); + let mut amount = Attribute::new("amount_total"); + amount.type_name = Some("decimal".into()); + original.attributes.push(amount); + + let ddl = emit_clickhouse_ddl(&[original]); + // Emit must produce backtick-quoted form (per the existing + // emit_quotes_non_bare_table_name test). + assert!( + ddl.contains("CREATE TABLE `sale.order` ("), + "expected backtick-quoted CREATE TABLE, got: {ddl}" + ); + + let recovered = parse_clickhouse_ddl(&ddl).expect("parse OK"); + assert_eq!(recovered.len(), 1); + assert_eq!(recovered[0].name, "sale.order"); + assert_eq!(recovered[0].attributes.len(), 1); + assert_eq!(recovered[0].attributes[0].name, "amount_total"); + assert_eq!(recovered[0].attributes[0].type_name.as_deref(), Some("decimal")); + } + + #[cfg(feature = "clickhouse-parser")] + #[test] + fn round_trip_three_segment_odoo_name() { + // `account.move.line` — three-segment dotted name backtick- + // quoted as a single identifier (no qualified-db semantics). + let c = Class::new("account.move.line"); + let ddl = emit_clickhouse_ddl(&[c]); + assert!( + ddl.contains("`account.move.line`"), + "three-segment name must be quoted, got: {ddl}" + ); + let recovered = parse_clickhouse_ddl(&ddl).expect("parse OK"); + assert_eq!(recovered.len(), 1); + assert_eq!(recovered[0].name, "account.move.line"); + } + // ── Round-trip tests (the load-bearing ones) ────────────────────── #[cfg(feature = "clickhouse-parser")]