---
phase: 06-tech-debt-cleanup
plan: "01"
type: execute
wave: 1
depends_on: []
files_modified:
  - src/query/table_function.rs
  - src/lib.rs
  - src/catalog.rs
  - test/sql/phase2_ddl.test
autonomous: true
requirements: []
gap_closure: true

must_haves:
  truths:
    - "No #[allow(dead_code)] annotations exist in table_function.rs"
    - "pub mod query in lib.rs is gated with #[cfg(feature = \"extension\")] consistent with pub mod ddl"
    - "cargo test passes under default features (bundled) after feature-gate change"
    - "cargo test sidecar_round_trip passes in sandboxed environments where TMPDIR is set"
    - "cargo test pragma_database_list_returns_file_path passes in sandboxed environments"
    - "cargo test init_catalog_loads_from_sidecar passes in sandboxed environments"
    - "phase2_ddl.test restart section can be re-run without hanging due to leftover sidecar state"
  artifacts:
    - path: "src/query/table_function.rs"
      provides: "Clean table function with no dead code"
      contains: "SemanticViewBindData"
    - path: "src/lib.rs"
      provides: "Consistent feature-gating for all extension-only modules"
      contains: "cfg(feature = \"extension\")"
    - path: "src/catalog.rs"
      provides: "Portable temp paths in sidecar tests using std::env::temp_dir()"
      contains: "temp_dir()"
    - path: "test/sql/phase2_ddl.test"
      provides: "Idempotent restart section that cleans up leftover state"
      contains: "drop_semantic_view"
  key_links:
    - from: "src/query/table_function.rs"
      to: "src/lib.rs"
      via: "query module gated at lib.rs level"
      pattern: "cfg.*feature.*extension.*query"
    - from: "src/catalog.rs"
      to: "std::env::temp_dir"
      via: "portable temp path resolution"
      pattern: "temp_dir"
---

<objective>
Eliminate dead code in table_function.rs, fix feature-gate inconsistency for pub mod query in lib.rs, make catalog sidecar tests portable across sandboxed environments, and make the SQLLogicTest restart section idempotent.

Purpose: Close tech debt identified by the v1.0 milestone audit -- no runtime behavior changes, only code hygiene and test reliability improvements.
Output: Four files modified with no new functionality, passing cargo test and consistent feature-gating.
</objective>

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

<context>
@.planning/PROJECT.md
@.planning/ROADMAP.md
@.planning/STATE.md
@.planning/phases/06-tech-debt-cleanup/06-RESEARCH.md
@.planning/v1.0-MILESTONE-AUDIT.md
@src/query/table_function.rs
@src/lib.rs
@src/catalog.rs
@src/query/mod.rs
@test/sql/phase2_ddl.test

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

From src/query/table_function.rs (lines 47-55, current state with dead code):
```rust
pub struct SemanticViewBindData {
    expanded_sql: String,
    column_names: Vec<String>,
    #[allow(dead_code)]
    column_type_ids: Vec<ffi::duckdb_type>,  // REMOVE this field
}
```

From src/query/table_function.rs (lines 103-135):
```rust
#[allow(dead_code)]
fn logical_type_from_duckdb_type(ty: ffi::duckdb_type) -> LogicalTypeHandle { ... }
// REMOVE this entire function
```

From src/query/table_function.rs (lines 258-274, bind call site):
```rust
let (column_names, column_type_ids) =
    infer_schema_or_default(state.conn, &expanded_sql, &dimensions, &metrics, &def);
// ...
Ok(SemanticViewBindData {
    expanded_sql,
    column_names,
    column_type_ids,  // REMOVE this field
})
```

From src/query/table_function.rs (lines 392-432, infer_schema_or_default):
```rust
fn infer_schema_or_default(...) -> (Vec<String>, Vec<ffi::duckdb_type>) { ... }
// Change return type to Vec<String>, remove type computation
```

From src/lib.rs (lines 1-11):
```rust
pub mod catalog;
pub mod expand;
pub mod model;
pub mod query;           // NOT gated -- must add #[cfg(feature = "extension")]

#[cfg(feature = "extension")]
pub mod ddl;             // Correctly gated
```

From src/query/mod.rs (ALL contents already gated):
```rust
#[cfg(feature = "extension")]
pub mod error;
#[cfg(feature = "extension")]
pub mod explain;
#[cfg(feature = "extension")]
pub mod table_function;
```

From src/catalog.rs (tests with hardcoded /tmp/):
```rust
// Line 250: let tmpfile = "/tmp/test_pragma_rust_check.duckdb";
// Line 306: let db_path = "/tmp/test_sidecar_roundtrip.duckdb";
// Line 330: let db_path = "/tmp/test_init_sidecar.duckdb";
```
</interfaces>
</context>

<tasks>

<task type="auto">
  <name>Task 1: Remove dead code from table_function.rs and refactor infer_schema_or_default to return only column names</name>
  <files>
    src/query/table_function.rs
  </files>
  <action>
Make four targeted edits to `src/query/table_function.rs`:

1. **Remove `column_type_ids` field from `SemanticViewBindData`** (lines 52-54):
   Delete the `#[allow(dead_code)]` annotation and the `column_type_ids: Vec<ffi::duckdb_type>` field.
   Also remove the doc comment on line 52 ("Output column types as DuckDB C API type enums (stored for error context).").
   After edit, the struct should be:
   ```rust
   pub struct SemanticViewBindData {
       expanded_sql: String,
       column_names: Vec<String>,
   }
   ```

2. **Remove `logical_type_from_duckdb_type` function entirely** (lines 103-135):
   Delete the entire function including its doc comment (starting from "/// Create a `LogicalTypeHandle`..." on line 103). This removes the `#[allow(dead_code)]` annotation.

3. **Update the bind call site** (lines 258-274):
   Change the destructuring from `let (column_names, column_type_ids) = infer_schema_or_default(...)` to `let column_names = infer_schema_or_default(...)`.
   Remove `column_type_ids` from the `SemanticViewBindData` constructor.

4. **Refactor `infer_schema_or_default`** (lines 392-432):
   - Change return type from `(Vec<String>, Vec<ffi::duckdb_type>)` to `Vec<String>`.
   - In the success path (line 403-405): change `if let Some((names, types)) = inferred { return (names, types); }` to `if let Some((names, _types)) = inferred { return names; }`.
   - In the fallback path: remove `column_types` vector entirely. Remove all `column_types.push(...)` lines. Change final return from `(column_names, column_types)` to `column_names`.

5. **Leave `try_infer_schema` unchanged** (lines 441-466):
   This function still returns `Option<(Vec<String>, Vec<ffi::duckdb_type>)>` internally. The type computation is cheap, side-effect-free, and needed to extract column names from the result. Minimal change: discard the types at the caller (`infer_schema_or_default`) with `_types`. Do NOT refactor `try_infer_schema` -- that is out of scope.

After these edits, confirm there are zero `#[allow(dead_code)]` annotations remaining in the file by searching.
  </action>
  <verify>
    <automated>cd /Users/paul/Documents/Dev/Personal/duckdb-semantic-views && grep -c "allow(dead_code)" src/query/table_function.rs | grep -q "^0$" && echo "PASS: no dead_code annotations" || echo "FAIL: dead_code annotations remain"</automated>
  </verify>
  <done>
    - SemanticViewBindData has exactly two fields: expanded_sql and column_names
    - logical_type_from_duckdb_type function is completely removed
    - infer_schema_or_default returns Vec<String> (not a tuple)
    - Zero #[allow(dead_code)] annotations in table_function.rs
    - try_infer_schema is unchanged (types discarded by caller with _types)
  </done>
</task>

<task type="auto">
  <name>Task 2: Fix feature-gate for pub mod query, make catalog tests portable, and make SQLLogicTest restart idempotent</name>
  <files>
    src/lib.rs
    src/catalog.rs
    test/sql/phase2_ddl.test
  </files>
  <action>
Three independent file edits:

**A. Feature-gate `pub mod query` in `src/lib.rs` (line 4):**

Change:
```rust
pub mod query;
```
To:
```rust
#[cfg(feature = "extension")]
pub mod query;
```

This is safe because `query/mod.rs` already gates ALL three submodules (`error`, `explain`, `table_function`) behind `#[cfg(feature = "extension")]`. No code under default features imports from `crate::query` -- this was verified during research.

**B. Make catalog sidecar tests use portable temp paths in `src/catalog.rs`:**

Edit three test functions to replace hardcoded `/tmp/` with `std::env::temp_dir()`:

1. `pragma_database_list_returns_file_path` (around line 249-270):
   Replace `let tmpfile = "/tmp/test_pragma_rust_check.duckdb";` with:
   ```rust
   let tmp = std::env::temp_dir();
   let tmpfile_buf = tmp.join("test_pragma_rust_check.duckdb");
   let tmpfile = tmpfile_buf.to_str().expect("temp dir is UTF-8");
   ```

2. `sidecar_round_trip` (around line 305-326):
   Replace `let db_path = "/tmp/test_sidecar_roundtrip.duckdb";` with:
   ```rust
   let tmp = std::env::temp_dir();
   let db_path_buf = tmp.join("test_sidecar_roundtrip.duckdb");
   let db_path = db_path_buf.to_str().expect("temp dir is UTF-8");
   ```

3. `init_catalog_loads_from_sidecar` (around line 329-365):
   Replace `let db_path = "/tmp/test_init_sidecar.duckdb";` with:
   ```rust
   let tmp = std::env::temp_dir();
   let db_path_buf = tmp.join("test_init_sidecar.duckdb");
   let db_path = db_path_buf.to_str().expect("temp dir is UTF-8");
   ```
   Also update `let _ = std::fs::remove_file(format!("{db_path}.wal"));` -- this still works because `db_path` is now a `&str` from the `PathBuf`.

NOTE: Do NOT change the `sidecar_path_derivation` test (line 289-302). That test uses hardcoded `/tmp/` paths as input to the `sidecar_path()` function to verify path derivation logic. Those are not files being created -- they are string inputs to a pure function. Changing them would weaken the test.

**C. Make SQLLogicTest restart section idempotent in `test/sql/phase2_ddl.test`:**

Insert cleanup logic AFTER the `load __TEST_DIR__/restart_test.db` line (line 158) and BEFORE the `statement ok` that defines the view (line 161).

Add this block:
```sql
# Idempotent cleanup: if a previous run left state behind via the sidecar file,
# init_catalog will have already loaded the view. Drop it before re-defining.
statement ok
SELECT CASE WHEN (SELECT count(*) FROM list_semantic_views() WHERE name = 'restart_test') > 0
       THEN drop_semantic_view('restart_test')
       ELSE 'no-op'
END;
```

This uses a CASE expression with a count() subquery to conditionally call `drop_semantic_view` only if the view exists. DuckDB evaluates CASE lazily, so `drop_semantic_view` is NOT called when count is 0.

If the CASE approach does not work (DuckDB may eagerly evaluate both branches), use this fallback approach instead: change the `define_semantic_view` call to first attempt a drop (tolerate error), then define:
```sql
# Tolerate error if view doesn't exist from a previous run
statement maybe
SELECT drop_semantic_view('restart_test');

statement ok
SELECT define_semantic_view('restart_test', ...);
```

If `statement maybe` is not supported by the DuckDB SQLLogicTest runner, the executor should investigate what the runner supports and use the simplest approach that works. The goal is: section 10 succeeds whether or not `restart_test.db.semantic_views` exists from a previous run.
  </action>
  <verify>
    <automated>cd /Users/paul/Documents/Dev/Personal/duckdb-semantic-views && cargo test --lib 2>&1 | tail -5</automated>
  </verify>
  <done>
    - pub mod query in lib.rs has #[cfg(feature = "extension")] attribute
    - All three catalog sidecar tests use std::env::temp_dir() instead of hardcoded /tmp/
    - sidecar_path_derivation test is unchanged (pure function test, not affected)
    - phase2_ddl.test section 10 has cleanup logic before define_semantic_view
    - cargo test passes under default features (bundled)
    - cargo build --no-default-features --features extension compiles successfully
  </done>
</task>

</tasks>

<verification>
1. `cargo test --lib` passes (all unit tests including catalog sidecar tests)
2. `grep -c "allow(dead_code)" src/query/table_function.rs` returns 0
3. `grep "cfg(feature.*extension" src/lib.rs` shows gating on both `pub mod ddl` and `pub mod query`
4. `grep "temp_dir" src/catalog.rs` shows portable temp paths in tests
5. `grep -A2 "load __TEST_DIR__/restart_test.db" test/sql/phase2_ddl.test` shows cleanup logic
6. `cargo build --no-default-features --features extension` compiles without errors (extension build still works)
</verification>

<success_criteria>
- Zero #[allow(dead_code)] annotations in table_function.rs
- pub mod query is feature-gated consistently with pub mod ddl
- All 3 catalog sidecar tests pass in both local and sandboxed environments
- phase2_ddl.test restart section is idempotent (no hang on re-run)
- No runtime behavior changes -- all modifications are code hygiene and test reliability
</success_criteria>

<output>
After completion, create `.planning/phases/06-tech-debt-cleanup/06-01-SUMMARY.md`
</output>
