---
phase: quick-260322-1zx
plan: 01
type: execute
wave: 1
depends_on: []
files_modified:
  - src/parse.rs
  - src/ddl/define.rs
  - test/sql/phase33_cardinality_inference.test
autonomous: true
requirements: [PK-OPTIONAL]

must_haves:
  truths:
    - "Tables declared without PRIMARY KEY in TABLES clause still work when the physical table has a PK constraint in DuckDB catalog"
    - "Tables declared without PRIMARY KEY that have no catalog PK still produce the existing error message"
    - "Tables with explicit PRIMARY KEY in TABLES clause continue to work unchanged"
    - "PK resolution from catalog fills in ref_columns so REFERENCES without explicit columns still works"
    - "Cardinality inference works correctly after catalog PK resolution"
  artifacts:
    - path: "src/parse.rs"
      provides: "Tolerant infer_cardinality that defers to bind-time when target PK is empty"
      contains: "pub(crate) fn infer_cardinality"
    - path: "src/ddl/define.rs"
      provides: "Catalog PK resolution step in DefineFromJsonVTab::bind before Phase 33 guard and graph validation"
      contains: "resolve_pk_from_catalog"
    - path: "test/sql/phase33_cardinality_inference.test"
      provides: "End-to-end sqllogictest covering PK-less TABLES with catalog PK"
  key_links:
    - from: "src/ddl/define.rs"
      to: "src/parse.rs"
      via: "calls infer_cardinality after catalog PK resolution"
      pattern: "infer_cardinality"
    - from: "src/ddl/define.rs"
      to: "duckdb_constraints()"
      via: "SQL query on persist_conn"
      pattern: "duckdb_constraints"
---

<objective>
Make PRIMARY KEY declarations optional in the TABLES clause of CREATE SEMANTIC VIEW.

When a table is declared without PRIMARY KEY and a relationship REFERENCES it without explicit columns, the extension should look up the PK from DuckDB's catalog metadata (`duckdb_constraints()`) at bind time (when a DB connection is available). If the catalog has no PK for that table, the existing error path triggers as before.

Purpose: Reduces boilerplate -- users should not have to redeclare PKs that DuckDB already knows about.
Output: Modified parse.rs (tolerant inference), modified define.rs (catalog PK resolution), updated sqllogictest.
</objective>

<execution_context>
@~/.claude/get-shit-done/workflows/execute-plan.md
@~/.claude/get-shit-done/templates/summary.md
</execution_context>

<context>
@CLAUDE.md
@src/parse.rs (infer_cardinality function at line 518)
@src/ddl/define.rs (DefineFromJsonVTab::bind at line 100)
@src/model.rs (TableRef, Join, SemanticViewDefinition)
@src/graph.rs (validate_graph, validate_fk_references)
@test/sql/phase33_cardinality_inference.test

<interfaces>
<!-- Key types and contracts the executor needs. -->

From src/model.rs:
```rust
pub struct TableRef {
    pub alias: String,
    pub table: String,
    pub pk_columns: Vec<String>,       // empty = no PK declared
    pub unique_constraints: Vec<Vec<String>>,
}

pub struct Join {
    pub table: String,         // target alias (REFERENCES target)
    pub from_alias: String,    // source alias
    pub fk_columns: Vec<String>,
    pub ref_columns: Vec<String>,  // empty = needs resolution from target PK
    pub name: Option<String>,
    pub cardinality: Cardinality,
    // ... other fields
}

pub struct SemanticViewDefinition {
    pub base_table: String,
    pub tables: Vec<TableRef>,
    pub joins: Vec<Join>,
    // ... other fields
}
```

From src/parse.rs:
```rust
// Currently private, needs pub(crate):
fn infer_cardinality(tables: &[TableRef], relationships: &mut [Join]) -> Result<(), ParseError>;
```

From src/ddl/define.rs:
```rust
pub struct DefineState {
    pub catalog: CatalogState,
    pub persist_conn: Option<ffi::duckdb_connection>,
    pub or_replace: bool,
    pub if_not_exists: bool,
}
// DefineFromJsonVTab::bind has access to state.persist_conn
```

From src/query/table_function.rs:
```rust
pub(crate) unsafe fn execute_sql_raw(
    conn: ffi::duckdb_connection,
    sql: &str,
) -> Result<ffi::duckdb_result, String>;
```
</interfaces>
</context>

<tasks>

<task type="auto">
  <name>Task 1: Make infer_cardinality tolerant and add catalog PK resolution in define.rs</name>
  <files>src/parse.rs, src/ddl/define.rs</files>
  <action>
Two changes:

**src/parse.rs -- Make `infer_cardinality` tolerant of missing PK:**

1. Change visibility from `fn infer_cardinality` to `pub(crate) fn infer_cardinality` so it can be called from `define.rs`.

2. In the `infer_cardinality` function, Step 1 (resolve ref_columns), change the error case at line 544-552:
   - Currently: when target exists but has empty `pk_columns`, it returns `Err(ParseError)`.
   - New behavior: when target exists but has empty `pk_columns`, **skip** this relationship (continue to next join). Leave `ref_columns` empty -- it will be resolved at bind time after catalog PK lookup. Do NOT error.
   - The `None` (target not found) case remains unchanged (caught by graph validation later).

3. In Step 2 (FK/ref column count check), the existing guard `!join.ref_columns.is_empty()` already handles the deferred case correctly -- it skips when ref_columns is empty.

4. In Step 3 (cardinality inference), the existing code only runs when `source` is found. Since ref_columns may be empty (deferred), cardinality will default to `ManyToOne` -- correct for the initial serialization. Cardinality will be re-inferred at bind time.

**src/ddl/define.rs -- Add catalog PK resolution and reorder the bind sequence:**

CRITICAL ORDERING: The new catalog PK resolution and re-inference MUST run BEFORE the Phase 33 old-format guard. The tolerant `infer_cardinality` now produces joins with `fk_columns` set but `ref_columns` empty for PK-optional tables. This is the same state the Phase 33 guard checks for. The resolution step fills in the PK from catalog, so after resolution any join that STILL has empty `ref_columns` is genuinely old-format (no catalog PK available either) -- the guard then fires correctly.

1. Add a new function `resolve_pk_from_catalog` that:
   - Takes `conn: ffi::duckdb_connection` and `def: &mut SemanticViewDefinition`
   - Iterates `def.tables` looking for entries with empty `pk_columns`
   - For each such table, queries: `SELECT constraint_column_names FROM duckdb_constraints() WHERE database_name = current_database() AND schema_name = 'main' AND table_name = '{table}' AND constraint_type = 'PRIMARY KEY'`
   - Uses `execute_sql_raw` (from `crate::query::table_function`) to run the query on the given connection
   - Parses the result: `constraint_column_names` returns a DuckDB LIST of VARCHAR. Use `duckdb_value_varchar` to get the string representation (e.g., `[id]` or `[first_name, last_name]`), then parse the bracket-delimited list into a `Vec<String>`.
   - Sets `table.pk_columns` to the parsed column names.
   - If the query fails or returns no rows, leaves `pk_columns` empty (graceful fallback -- the existing error from `infer_cardinality` will trigger when it is re-run, or `validate_fk_references` will catch it).

2. In `DefineFromJsonVTab::bind`, REORDER the bind sequence. The current code (lines 104-127) runs:
   (a) deserialize JSON (line 112-113)
   (b) Phase 33 guard (line 116-123)
   (c) validate_graph (line 127)
   ... and `state` access is much later (line 141).

   The new sequence MUST be:
   (a) Deserialize JSON (unchanged, line 112-113)
   (b) MOVE the `state` access UP from line 141 to immediately after deserialization:
       ```rust
       let state_ptr = bind.get_extra_info::<DefineState>();
       let state = unsafe { &*state_ptr };
       ```
   (c) Resolve PKs from catalog (NEW):
       ```rust
       // Resolve PKs from catalog for tables without explicit PRIMARY KEY.
       if let Some(conn) = state.persist_conn {
           resolve_pk_from_catalog(conn, &mut def);
       }
       ```
   (d) Re-run cardinality inference (NEW):
       ```rust
       // Re-run cardinality inference now that catalog PKs are resolved.
       // Any joins with still-empty ref_columns (no catalog PK found) will
       // be caught by the Phase 33 guard or validate_fk_references below.
       crate::parse::infer_cardinality(&def.tables, &mut def.joins)
           .map_err(|e| Box::<dyn std::error::Error>::from(e.message))?;
       ```
   (e) Phase 33 old-format guard (MOVED DOWN from its current position):
       ```rust
       // Phase 33: Reject old-format JSON that has FK columns but no ref_columns.
       // NOW SAFE: catalog PK resolution has run, so any remaining empty
       // ref_columns genuinely means no PK exists for the target table.
       for join in &def.joins {
           if !join.fk_columns.is_empty() && join.ref_columns.is_empty() {
               return Err(Box::<dyn std::error::Error>::from(
                   "This semantic view was created with an older version. \
                    Please recreate it with the new DDL syntax.",
               ));
           }
       }
       ```
   (f) validate_graph (unchanged, line 127)
   (g) validate_facts, validate_derived_metrics, validate_using_relationships (unchanged)
   (h) The rest of the bind function continues as before. Remove the duplicate `state_ptr`/`state` lines from their old location (line 141-142) since they are now earlier.

   Note: `infer_cardinality` returns `Result<(), ParseError>` but `bind` expects `Box<dyn Error>`. Map the error via `.map_err(|e| Box::<dyn std::error::Error>::from(e.message))`.

3. The `resolve_pk_from_catalog` function should handle the `duckdb_value_varchar` result parsing carefully. The LIST column from `duckdb_constraints()` when read via `duckdb_value_varchar` returns a string like `[col1, col2]`. Parse by: trimming `[` and `]`, splitting on `, `, trimming whitespace from each element.

4. For in-memory databases (`persist_conn` is None), the catalog lookup is skipped. Tables must have explicit PKs declared, same as today. This is documented behavior since in-memory DBs have no persistent schema.

5. Import `ParseError` from `crate::parse` in define.rs (it is already `pub`).
  </action>
  <verify>
    <automated>cd /Users/paul/Documents/Dev/Personal/duckdb-semantic-views && cargo test -- --test-threads=1 2>&amp;1 | tail -5</automated>
  </verify>
  <done>
    - `infer_cardinality` is `pub(crate)` and skips (not errors) when target has no PK
    - `resolve_pk_from_catalog` function exists in define.rs
    - DefineFromJsonVTab::bind sequence is: deserialize -> state access -> resolve_pk_from_catalog -> infer_cardinality -> Phase 33 guard -> validate_graph
    - All existing cargo tests pass (no regression)
  </done>
</task>

<task type="auto">
  <name>Task 2: Add Rust unit tests for catalog PK resolution</name>
  <files>src/parse.rs, src/ddl/define.rs</files>
  <action>
**src/parse.rs -- Add test for tolerant behavior:**

In the `#[cfg(test)] mod tests` section of parse.rs (if one exists; otherwise in existing cardinality tests), add a test:

1. `infer_cardinality_skips_empty_pk_target`: Create tables where the REFERENCES target has empty `pk_columns`. Verify `infer_cardinality` returns `Ok(())` and leaves `ref_columns` empty on the join (does not error). This tests the new tolerant behavior.

2. `infer_cardinality_still_resolves_when_pk_present`: Verify existing behavior unchanged -- target with PK still resolves ref_columns correctly.

**src/ddl/define.rs -- Test the parse helper for constraint_column_names parsing:**

The bracket-delimited list parser (`[col1, col2]` -> `Vec<String>`) should be extracted into a small testable helper function (e.g., `parse_constraint_columns(s: &str) -> Vec<String>`). Add tests:

1. `parse_constraint_columns_single`: `"[id]"` -> `vec!["id"]`
2. `parse_constraint_columns_composite`: `"[first_name, last_name]"` -> `vec!["first_name", "last_name"]`
3. `parse_constraint_columns_empty`: `"[]"` -> `vec![]`
4. `parse_constraint_columns_no_brackets`: `""` -> `vec![]` (graceful fallback)

Note: The full integration test of `resolve_pk_from_catalog` requires a DuckDB connection, which is tested via sqllogictest in Task 3. The unit tests here focus on the parsing logic and the tolerant `infer_cardinality` behavior.
  </action>
  <verify>
    <automated>cd /Users/paul/Documents/Dev/Personal/duckdb-semantic-views && cargo test -- --test-threads=1 2>&amp;1 | tail -5</automated>
  </verify>
  <done>
    - Unit test for tolerant infer_cardinality passes
    - Unit tests for parse_constraint_columns helper pass
    - All existing tests still pass
  </done>
</task>

<task type="auto">
  <name>Task 3: Add sqllogictest for end-to-end PK-optional flow</name>
  <files>test/sql/phase33_cardinality_inference.test</files>
  <action>
Append new test cases to the existing `test/sql/phase33_cardinality_inference.test` file (at the end, after existing tests). Use table name prefix `p33_pkopt_` to avoid collisions.

**Test cases to add:**

1. **PK from catalog -- basic**: Create a physical table with PRIMARY KEY constraint (`CREATE TABLE p33_pkopt_customers (id INTEGER PRIMARY KEY, name VARCHAR)`). Create a semantic view where the TABLES clause declares this table WITHOUT PRIMARY KEY (`c AS p33_pkopt_customers`). Add a RELATIONSHIPS clause that REFERENCES c (without explicit columns). Verify the view creates successfully and a query returns correct results.

2. **PK from catalog -- composite PK**: Create a physical table with composite PK (`CREATE TABLE p33_pkopt_shipments (order_id INTEGER, line_id INTEGER, status VARCHAR, PRIMARY KEY (order_id, line_id))`). Create a semantic view using it without PK in TABLES. Add a REFERENCES with matching composite FK columns. Verify creation and query.

3. **Explicit PK still works**: Create a semantic view with explicit PRIMARY KEY in TABLES clause alongside a table that also has a catalog PK. Verify the explicit DDL PK is used (existing behavior preserved).

4. **No catalog PK -- error**: Create a physical table WITHOUT a PRIMARY KEY constraint (`CREATE TABLE p33_pkopt_logs (msg VARCHAR, ts TIMESTAMP)`). Create a semantic view referencing it without PK in TABLES and without explicit ref columns in REFERENCES. Verify the expected error message fires: "has no PRIMARY KEY".

5. **Mixed -- some with PK, some without**: A multi-table semantic view where one table has explicit PK in DDL and another relies on catalog PK lookup. Verify both work correctly together.

6. **DESCRIBE output**: After creating a view with catalog-resolved PK, run DESCRIBE and verify the relationship shows correct cardinality (ManyToOne for standard FK patterns).

Each test should clean up with `DROP SEMANTIC VIEW` and `DROP TABLE` statements.

Build and run:
```bash
just build && just test-sql
```
  </action>
  <verify>
    <automated>cd /Users/paul/Documents/Dev/Personal/duckdb-semantic-views && just build && just test-sql 2>&amp;1 | tail -10</automated>
  </verify>
  <done>
    - All new sqllogictest cases pass (PK from catalog, composite PK, explicit PK, error case, mixed, DESCRIBE)
    - All existing sqllogictest cases still pass
    - `just test-all` passes (full quality gate)
  </done>
</task>

</tasks>

<verification>
Run the full quality gate to confirm no regressions:

```bash
just test-all
```

This runs: cargo test (Rust unit + proptest + doc tests), just test-sql (sqllogictest), and just test-ducklake-ci (DuckLake integration).
</verification>

<success_criteria>
1. A CREATE SEMANTIC VIEW with a table declared without PRIMARY KEY succeeds when the physical table has a PK constraint in DuckDB
2. REFERENCES without explicit columns resolves correctly via catalog PK lookup
3. Cardinality inference works after catalog PK resolution
4. Tables without catalog PK still produce the existing error
5. Explicit PRIMARY KEY in DDL continues to work unchanged (no regression)
6. `just test-all` passes
</success_criteria>

<output>
After completion, create `.planning/quick/260322-1zx-make-primary-key-optional-by-referring-t/260322-1zx-SUMMARY.md`
</output>
