---
phase: 42-refactor-tidy-ups-and-test-reorganisation
plan: 03
type: execute
wave: 2
depends_on: ["42-01"]
files_modified:
  - test/sql/phase42_persistence.test
  - test/sql/TEST_LIST
  - src/query/table_function.rs
  - src/util.rs
autonomous: true
requirements: [TIDY-06, TIDY-07, TIDY-08]

must_haves:
  truths:
    - "A file-backed database round-trip test verifies that semantic views persist across restart"
    - "A layout guard test fails loudly if Value and duckdb_value sizes diverge"
    - "suggest_closest property test verifies suggestions are always valid names from the input set"
  artifacts:
    - path: "test/sql/phase42_persistence.test"
      provides: "File-backed catalog round-trip sqllogictest"
      contains: "restart"
    - path: "test/sql/TEST_LIST"
      provides: "Updated test list including phase42_persistence.test"
      contains: "phase42_persistence.test"
    - path: "src/query/table_function.rs"
      provides: "Transmute layout guard test"
      contains: "fn value_layout_matches_duckdb_value"
    - path: "src/util.rs"
      provides: "suggest_closest property tests"
      contains: "proptest!"
  key_links:
    - from: "test/sql/phase42_persistence.test"
      to: "src/ddl/persist.rs"
      via: "exercises parameterized CREATE/DROP/SHOW through full extension load"
      pattern: "CREATE SEMANTIC VIEW"
    - from: "src/query/table_function.rs"
      to: "duckdb::vtab::Value"
      via: "size_of/align_of assertions"
      pattern: "size_of.*Value"
---

<objective>
Add three new test coverage items: a file-backed persistence round-trip sqllogictest (exercising CREATE/restart/SHOW), a transmute layout guard that catches Value/duckdb_value size mismatches at compile-test time, and property tests for suggest_closest ensuring suggestions are always valid names.

Purpose: Close the three coverage gaps identified in the code review -- persistence round-trip, unsafe transmute fragility, and error suggestion correctness.
Output: New phase42_persistence.test, layout guard test in table_function.rs, proptests in util.rs.
</objective>

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

<context>
@.planning/PROJECT.md
@.planning/ROADMAP.md
@.planning/STATE.md
@.planning/phases/42-refactor-tidy-ups-and-test-reorganisation/42-RESEARCH.md
@test/sql/phase2_restart.test
@test/sql/TEST_LIST
@src/query/table_function.rs
@src/util.rs

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

From test/sql/phase2_restart.test (the existing restart test pattern):
```sql
require semantic_views
require notwindows
load __TEST_DIR__/restart_test.db

statement ok
CREATE SEMANTIC VIEW restart_test AS
TABLES (...)
DIMENSIONS (...)
METRICS (...)

query T rowsort
SELECT name FROM list_semantic_views();
----
restart_test

restart

query T rowsort
SELECT name FROM list_semantic_views();
----
restart_test
```
NOTE: phase2_restart.test is EXCLUDED from TEST_LIST because the Python sqllogictest
runner cannot reload an external extension after the `restart` directive. The new test
will have the same limitation -- it should also be excluded from TEST_LIST but documented.

CORRECTION: Re-read the phase2_restart.test header carefully. It says:
"the Python sqllogictest runner cannot reload an external extension after the `restart`
directive in a file-backed database". This means the new test also CANNOT be in TEST_LIST.
Instead, add it alongside phase2_restart.test as a documented but manually-runnable test.

From src/query/table_function.rs (the transmute at lines 130-148):
```rust
pub(crate) unsafe fn value_raw_ptr(value: &Value) -> ffi::duckdb_value {
    let ptr_to_value = std::ptr::from_ref(value).cast::<ffi::duckdb_value>();
    std::ptr::read(ptr_to_value)
}
```
- `duckdb::vtab::Value` is available under the default `bundled` feature
- `ffi::duckdb_value` is `*mut c_void` (pointer width)
- The test needs `use duckdb::vtab::Value` and `use libduckdb_sys as ffi`

From src/util.rs (the suggest_closest function):
```rust
pub fn suggest_closest(name: &str, available: &[String]) -> Option<String> {
    let query = name.to_ascii_lowercase();
    let mut best: Option<(usize, &str)> = None;
    for candidate in available {
        let dist = strsim::levenshtein(&query, &candidate.to_ascii_lowercase());
        if dist <= 3 {
            // ... track best match
        }
    }
    best.map(|(_, s)| s.to_string())
}
```
- Returns `Some(name)` if best match has edit distance <= 3
- Returns `None` if no match is close enough
- Both query and candidates are lowercased for comparison
- The function signature takes `&[String]`, not `&[&str]`

Proptest is already a dev dependency (used in 13 other proptest blocks).
The existing `util.rs` tests module starts at line 77.
</interfaces>
</context>

<tasks>

<task type="auto">
  <name>Task 1: File-backed persistence round-trip test + transmute layout guard</name>
  <files>test/sql/phase42_persistence.test, src/query/table_function.rs</files>
  <read_first>
    - test/sql/phase2_restart.test (existing restart persistence test pattern -- read full file)
    - test/sql/TEST_LIST (current list of sqllogictest files)
    - src/query/table_function.rs lines 125-150 (the value_raw_ptr transmute and surrounding code)
  </read_first>
  <action>
**Part A: Create test/sql/phase42_persistence.test**

Create a new sqllogictest file that exercises the full persistence round-trip with the parameterized query infrastructure from Plan 01:

```sql
# Phase 42: File-backed persistence round-trip test (TIDY-06).
#
# Verifies that semantic view definitions persist across database restart
# via the parameterized prepared statement persistence layer. Exercises
# CREATE, SHOW, DROP, and DESCRIBE across a restart boundary.
#
# NOTE: This test is EXCLUDED from test/sql/TEST_LIST because the Python
# sqllogictest runner cannot reload an external extension after the
# `restart` directive in a file-backed database. Run manually:
#   sqllogictest test/sql/phase42_persistence.test
#
# Related: test/sql/phase2_restart.test (original persistence test)

require semantic_views

require notwindows

load __TEST_DIR__/phase42_persist.db

# Idempotent cleanup from previous run
statement ok
DROP SEMANTIC VIEW IF EXISTS persist_roundtrip;

# Create backing table
statement ok
CREATE TABLE events (type VARCHAR, n INTEGER);

# Define a semantic view (exercises persist_define with parameterized INSERT)
statement ok
CREATE SEMANTIC VIEW persist_roundtrip AS
TABLES (
    e AS events PRIMARY KEY (type)
)
DIMENSIONS (
    e.type AS event_type
)
METRICS (
    e.n AS event_count count(*)
)

# Verify the view exists before restart
query T
SELECT name FROM list_semantic_views();
----
persist_roundtrip

# Verify DESCRIBE works before restart
query TTTTT
DESCRIBE SEMANTIC VIEW persist_roundtrip;
----
(multiple rows expected -- just verify it doesn't error)

restart

# After restart: extension reloaded, definitions loaded from persisted table.
# The view must still be present.
query T
SELECT name FROM list_semantic_views();
----
persist_roundtrip

# Verify SHOW SEMANTIC DIMENSIONS works after restart
query TTTTTT
SHOW SEMANTIC DIMENSIONS IN persist_roundtrip;
----
(verify column count and that event_type dimension is present)

# Drop the view (exercises persist_drop with parameterized DELETE)
statement ok
DROP SEMANTIC VIEW persist_roundtrip;

# Verify it's gone
query I
SELECT count(*) FROM list_semantic_views();
----
0
```

IMPORTANT: After reading the existing phase2_restart.test in detail, adjust the query format and assertions to match the exact output patterns used in sqllogictest for this project. The above is a template -- the executor should:

1. Use `query T rowsort` instead of `query T` if the order is not guaranteed
2. Match the exact column type letters (T for text, I for integer, etc.) to the actual SHOW/DESCRIBE output schemas
3. For DESCRIBE, use the 5-column schema (object_kind, object_name, parent_entity, property, property_value) established in Phase 41
4. For SHOW SEMANTIC DIMENSIONS, use the 6-column schema (database_name, schema_name, semantic_view_name, table_name, name, data_type) established in Phase 40
5. Use `list_semantic_views()` (not SHOW SEMANTIC VIEWS) to avoid the created_on timestamp which varies per run

Do NOT add this file to test/sql/TEST_LIST -- it has the same `restart` limitation as phase2_restart.test.

**Part B: Add transmute layout guard test to src/query/table_function.rs**

Add a test at the end of the file (inside an existing `#[cfg(test)] mod tests` block, or create one). This test must compile under the default `bundled` feature (not `extension`):

```rust
#[cfg(test)]
mod tests {
    use super::*;

    /// Guard against duckdb crate version bumps that change Value layout.
    ///
    /// `value_raw_ptr` transmutes `duckdb::vtab::Value` to access its inner
    /// `duckdb_value` pointer. This test fails loudly if the sizes diverge,
    /// catching the bug before it manifests as undefined behavior.
    #[test]
    fn value_layout_matches_duckdb_value() {
        assert_eq!(
            std::mem::size_of::<duckdb::vtab::Value>(),
            std::mem::size_of::<ffi::duckdb_value>(),
            "Value size changed -- value_raw_ptr transmute is broken"
        );
        assert_eq!(
            std::mem::align_of::<duckdb::vtab::Value>(),
            std::mem::align_of::<ffi::duckdb_value>(),
            "Value alignment changed -- value_raw_ptr transmute is broken"
        );
    }
}
```

Check whether `src/query/table_function.rs` already has a `#[cfg(test)]` module. If it does, add the test inside it. If not, create one at the end of the file.

The test uses `duckdb::vtab::Value` (from the `duckdb` crate, available under `bundled` feature) and `ffi::duckdb_value` (from `libduckdb_sys`). Both should already be imported at the top of the file. Verify imports are available.

Note: `ffi::duckdb_value` is `*mut c_void`. Its size is `std::mem::size_of::<*mut c_void>()` which is the platform pointer width (8 bytes on 64-bit). `Value` should also be one pointer wide (single field: `ptr: duckdb_value`).
  </action>
  <verify>
    <automated>cd /Users/paul/Documents/Dev/Personal/duckdb-semantic-views && cargo test value_layout 2>&1 | tail -5</automated>
  </verify>
  <acceptance_criteria>
    - test/sql/phase42_persistence.test exists and contains `require semantic_views`, `load __TEST_DIR__/`, `restart`, `CREATE SEMANTIC VIEW`, and `DROP SEMANTIC VIEW`
    - test/sql/phase42_persistence.test is NOT listed in test/sql/TEST_LIST
    - test/sql/phase42_persistence.test header comment explains WHY it is excluded from TEST_LIST (restart limitation)
    - src/query/table_function.rs contains `fn value_layout_matches_duckdb_value()`
    - src/query/table_function.rs test contains `std::mem::size_of::<duckdb::vtab::Value>()`
    - src/query/table_function.rs test contains `std::mem::size_of::<ffi::duckdb_value>()`
    - `cargo test value_layout` exits 0
  </acceptance_criteria>
  <done>File-backed persistence round-trip test created. Transmute layout guard test catches Value/duckdb_value size or alignment mismatches.</done>
</task>

<task type="auto">
  <name>Task 2: Add suggest_closest property tests to util.rs</name>
  <files>src/util.rs</files>
  <read_first>
    - src/util.rs (full file -- 153 lines, see suggest_closest function at line 12 and existing tests at line 77)
  </read_first>
  <action>
Add proptest property tests to the existing `#[cfg(test)] mod tests` block in src/util.rs.

Add the proptest import at the top of the test module:
```rust
use proptest::prelude::*;
```

Add the following property tests after the existing `replace_word_boundary_*` tests (after line 152, before the closing `}` of the tests module):

```rust
    // -------------------------------------------------------------------
    // suggest_closest property tests
    // -------------------------------------------------------------------

    proptest! {
        /// Any suggestion returned by suggest_closest must be a member of the
        /// input `available` list. This prevents the function from inventing
        /// names that don't exist in the model.
        #[test]
        fn suggestion_is_always_valid_name(
            query in "[a-z_]{1,20}",
            names in prop::collection::vec("[a-z_]{1,20}", 1..20)
        ) {
            if let Some(suggestion) = suggest_closest(&query, &names) {
                prop_assert!(
                    names.contains(&suggestion),
                    "suggest_closest returned '{}' which is not in available names: {:?}",
                    suggestion,
                    names
                );
            }
        }

        /// An exact match (query == one of the available names) should always
        /// produce a suggestion, since edit distance is 0 which is within the
        /// threshold of 3.
        #[test]
        fn exact_match_always_suggests(
            name in "[a-z_]{1,20}",
            others in prop::collection::vec("[a-z_]{1,20}", 0..10)
        ) {
            let mut names = others;
            names.push(name.clone());
            let suggestion = suggest_closest(&name, &names);
            prop_assert!(
                suggestion.is_some(),
                "exact match '{}' should always produce a suggestion",
                name
            );
            prop_assert_eq!(
                suggestion.unwrap(),
                name,
                "exact match should suggest itself"
            );
        }

        /// When the available list is empty, suggest_closest must return None.
        #[test]
        fn empty_names_returns_none(
            query in "[a-z_]{1,20}"
        ) {
            let names: Vec<String> = vec![];
            prop_assert!(suggest_closest(&query, &names).is_none());
        }
    }
```

Ensure `proptest` is available as a dev dependency. Check Cargo.toml:
```bash
grep proptest Cargo.toml
```
It should already be there (13 proptest blocks exist in the codebase). If for some reason the import doesn't resolve, use the full path `proptest::proptest!` macro.

The property tests use `prop_assert!` and `prop_assert_eq!` (proptest macros) rather than `assert!` / `assert_eq!` because proptest needs its own assertion macros for shrinking to work correctly.
  </action>
  <verify>
    <automated>cd /Users/paul/Documents/Dev/Personal/duckdb-semantic-views && cargo test suggest 2>&1 | tail -10</automated>
  </verify>
  <acceptance_criteria>
    - src/util.rs contains `proptest!` macro invocation
    - src/util.rs contains `fn suggestion_is_always_valid_name(`
    - src/util.rs contains `fn exact_match_always_suggests(`
    - src/util.rs contains `fn empty_names_returns_none(`
    - src/util.rs contains `use proptest::prelude::*` inside the test module
    - `cargo test suggest` exits 0 (all three property tests pass, each running 256 cases)
    - `cargo test util` exits 0 (existing unit tests plus new proptests all pass)
  </acceptance_criteria>
  <done>suggest_closest property tested: suggestions are always valid names, exact matches always suggest, empty lists return None. Three proptest properties running 256 cases each.</done>
</task>

</tasks>

<verification>
After both tasks complete:
1. `cargo test value_layout` passes (layout guard)
2. `cargo test suggest` passes (proptests)
3. `cargo test` passes (all Rust unit tests including new tests)
4. `just build && just test-sql` passes (sqllogictest suite)
5. phase42_persistence.test file exists with restart round-trip
</verification>

<success_criteria>
- File-backed persistence round-trip test exercises CREATE/restart/SHOW/DROP (per TIDY-06)
- Transmute layout guard test catches size/alignment mismatches between Value and duckdb_value (per TIDY-07)
- suggest_closest proptests verify suggestions are always valid names (per TIDY-08)
- `just test-all` passes
</success_criteria>

<output>
After completion, create `.planning/phases/42-refactor-tidy-ups-and-test-reorganisation/42-03-SUMMARY.md`
</output>
