---
phase: 42-refactor-tidy-ups-and-test-reorganisation
plan: 02
type: execute
wave: 1
depends_on: []
files_modified:
  - src/model.rs
  - src/expand/test_helpers.rs
  - src/expand/sql_gen.rs
autonomous: true
requirements: [TIDY-03, TIDY-05]

must_haves:
  truths:
    - "The parallel Vec invariant (column_type_names[i] pairs with column_types_inferred[i]) is expressed via a typed helper method"
    - "sql_gen.rs test fixtures are extracted into expand/test_helpers.rs with shared builder functions"
    - "All 77 sql_gen.rs tests pass using the extracted fixtures"
  artifacts:
    - path: "src/model.rs"
      provides: "inferred_types() helper method on SemanticViewDefinition"
      contains: "fn inferred_types"
    - path: "src/expand/test_helpers.rs"
      provides: "Shared test fixture builders for expand module tests"
      contains: "pub(super) fn orders_view"
    - path: "src/expand/sql_gen.rs"
      provides: "Refactored tests using shared fixtures"
      contains: "use super::test_helpers::"
  key_links:
    - from: "src/expand/sql_gen.rs"
      to: "src/expand/test_helpers.rs"
      via: "use super::test_helpers::"
      pattern: "test_helpers::"
---

<objective>
Add a typed helper method for the parallel Vec invariant in model.rs, and extract the verbose inline test fixtures from sql_gen.rs (~1,800 lines of test code) into shared test_helpers.rs builders following the graph/test_helpers.rs pattern.

Purpose: Improve code readability (parallel Vec invariant made explicit) and reduce test maintenance burden (shared fixtures instead of 30-line struct literals per test).
Output: model.rs with inferred_types() method, expand/test_helpers.rs with builder functions, refactored sql_gen.rs tests.
</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
@src/model.rs
@src/expand/test_helpers.rs
@src/expand/sql_gen.rs
@src/graph/test_helpers.rs

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

From src/model.rs (the parallel Vec fields at lines 181-192):
```rust
pub struct SemanticViewDefinition {
    // ... other fields ...
    #[serde(default)]
    pub column_type_names: Vec<String>,
    #[serde(default)]
    pub column_types_inferred: Vec<u32>,
    // ... other fields ...
}
```

From src/graph/test_helpers.rs (the pattern to follow):
```rust
pub(super) fn make_def(
    tables: Vec<(&str, &str, Vec<&str>)>,
    joins: Vec<(&str, &str, Vec<&str>)>,
    dims: Vec<(&str, Option<&str>)>,
    metrics: Vec<(&str, Option<&str>)>,
) -> SemanticViewDefinition { ... }

pub(super) fn make_def_with_facts(
    tables: Vec<(&str, &str, Vec<&str>)>,
    joins: Vec<(&str, &str, Vec<&str>)>,
    facts: Vec<(&str, &str, Vec<(&str, &str)>)>,
    dims: Vec<(&str, Option<&str>)>,
    metrics: Vec<(&str, Option<&str>)>,
) -> SemanticViewDefinition { ... }
```

From src/expand/sql_gen.rs (the existing inline orders_view helper at line 228):
```rust
fn orders_view() -> SemanticViewDefinition {
    SemanticViewDefinition {
        base_table: "orders".to_string(),
        tables: vec![],
        dimensions: vec![
            Dimension { name: "region".into(), expr: "region".into(), source_table: None, output_type: None },
            Dimension { name: "status".into(), expr: "status".into(), source_table: None, output_type: None },
        ],
        metrics: vec![
            Metric { name: "total_revenue".into(), expr: "sum(amount)".into(), source_table: None, output_type: None, using_relationships: vec![] },
            Metric { name: "order_count".into(), expr: "count(*)".into(), source_table: None, output_type: None, using_relationships: vec![] },
        ],
        joins: vec![], facts: vec![],
        column_type_names: vec![], column_types_inferred: vec![],
        created_on: None, database_name: None, schema_name: None,
    }
}
```

From src/expand/sql_gen.rs (types used in tests):
```rust
use crate::expand::{expand, ExpandError, QueryRequest};
use crate::model::{Dimension, Fact, Join, Metric, SemanticViewDefinition, TableRef};
```
</interfaces>
</context>

<tasks>

<task type="auto">
  <name>Task 1: Add inferred_types() helper and build expand/test_helpers.rs fixtures</name>
  <files>src/model.rs, src/expand/test_helpers.rs</files>
  <read_first>
    - src/model.rs (see SemanticViewDefinition struct, particularly column_type_names and column_types_inferred at lines 181-192)
    - src/expand/test_helpers.rs (currently a 5-line placeholder)
    - src/graph/test_helpers.rs (reference pattern for shared test builders -- read full file)
    - src/expand/sql_gen.rs lines 220-400 (see the inline orders_view() helper and the first ~10 tests to understand common patterns)
    - src/expand/sql_gen.rs lines 400-800 (see multi-table join test fixtures)
  </read_first>
  <action>
**Part A: Add inferred_types() method to SemanticViewDefinition in src/model.rs.**

Add this method impl block after the struct definition (or inside an existing impl block if one exists):

```rust
impl SemanticViewDefinition {
    /// Iterate over inferred column types as (name, duckdb_type) pairs.
    ///
    /// Zips `column_type_names` and `column_types_inferred`, which are parallel
    /// vectors populated by DDL-time LIMIT 0 inference. Returns an empty
    /// iterator if inference did not run (both vecs empty).
    ///
    /// This method makes the parallel-vector invariant explicit: callers get
    /// typed pairs rather than indexing two vecs manually.
    pub fn inferred_types(&self) -> impl Iterator<Item = (&str, u32)> {
        self.column_type_names
            .iter()
            .map(String::as_str)
            .zip(self.column_types_inferred.iter().copied())
    }
}
```

Do NOT change the serde representation. Do NOT rename or remove the existing `column_type_names` / `column_types_inferred` fields. The helper method is an API addition only.

If an `impl SemanticViewDefinition` block already exists, add the method there. If not, create one.

**Part B: Build expand/test_helpers.rs with shared fixture functions.**

Replace the placeholder content of src/expand/test_helpers.rs with a full test helpers module. Analyze the test fixtures in sql_gen.rs to identify the common patterns:

1. **`orders_view()`** -- Single-table, 2 dimensions (region, status), 2 metrics (total_revenue = sum(amount), order_count = count(*)). Base table "orders". No joins, no facts. This is the most common base fixture.

2. **`orders_with_customers()`** -- Two tables: orders (base) + customers. Join from orders to customers on customer_id. Adds customer dimensions (customer_name from customers table). Used for multi-table join tests.

3. **Mutation methods** on SemanticViewDefinition for targeted test variations:
   - `with_dimension(name, expr, source_table)` -- add a dimension
   - `with_metric(name, expr, source_table)` -- add a metric
   - `with_join(from_alias, to_table, fk_columns)` -- add a join
   - `with_table(alias, table, pk_columns)` -- add a table ref
   - `with_fact(name, table, expr, derived_metrics)` -- add a fact
   - `with_using_relationship(metric_name, relationships)` -- set using_relationships on a named metric

The module should use `pub(super)` visibility (same as graph/test_helpers.rs) since it's only used within the expand module's test code.

Template for the file:

```rust
//! Shared test helpers for expand submodule tests.
//!
//! Provides builder functions for common SemanticViewDefinition fixtures,
//! following the pattern established in `graph/test_helpers.rs`.

use crate::model::{Dimension, Fact, Join, Metric, SemanticViewDefinition, TableRef};

/// Base orders view: single table, 2 dimensions, 2 metrics.
///
/// - base_table: "orders"
/// - dimensions: region, status
/// - metrics: total_revenue = sum(amount), order_count = count(*)
pub(super) fn orders_view() -> SemanticViewDefinition {
    SemanticViewDefinition {
        base_table: "orders".to_string(),
        tables: vec![],
        dimensions: vec![
            Dimension {
                name: "region".to_string(),
                expr: "region".to_string(),
                source_table: None,
                output_type: None,
            },
            Dimension {
                name: "status".to_string(),
                expr: "status".to_string(),
                source_table: None,
                output_type: None,
            },
        ],
        metrics: vec![
            Metric {
                name: "total_revenue".to_string(),
                expr: "sum(amount)".to_string(),
                source_table: None,
                output_type: None,
                using_relationships: vec![],
            },
            Metric {
                name: "order_count".to_string(),
                expr: "count(*)".to_string(),
                source_table: None,
                output_type: None,
                using_relationships: vec![],
            },
        ],
        joins: vec![],
        facts: vec![],
        column_type_names: vec![],
        column_types_inferred: vec![],
        created_on: None,
        database_name: None,
        schema_name: None,
    }
}

/// Orders + customers two-table view with a join.
///
/// Extends orders_view() with:
/// - tables: [orders PK(id), customers PK(id)]
/// - join: orders -> customers on customer_id
/// - additional dimension: customer_name from customers
pub(super) fn orders_with_customers() -> SemanticViewDefinition {
    // Build from scratch rather than mutating orders_view() to keep explicit
    // ... (fill in based on the actual multi-table test fixtures in sql_gen.rs)
}
```

Read through the sql_gen.rs tests to identify ALL distinct fixture patterns needed. Create helpers for each. Common variations to look for:
- Single-table with output_type on dimensions/metrics
- Multi-table with different join configurations (2-table, 3-table)
- Facts with derived metrics
- Role-playing dimensions (same table joined twice with different aliases)
- USING relationships on metrics

For each variation, create either a standalone fixture function OR a mutation method on SemanticViewDefinition. Prefer mutation methods when the variation is small (e.g., adding one fact to an existing fixture).

Mutation methods should be defined as a trait impl or as free functions that take and return SemanticViewDefinition, whichever is more ergonomic. The graph/test_helpers.rs uses standalone functions with tuple parameters. For expand, builder-style methods on SemanticViewDefinition are more readable:

```rust
/// Extension trait for test fixture mutations.
pub(super) trait TestFixtureExt {
    fn with_dimension(self, name: &str, expr: &str, source_table: Option<&str>) -> Self;
    fn with_metric(self, name: &str, expr: &str, source_table: Option<&str>) -> Self;
    fn with_join(self, from_alias: &str, to_table: &str, fk_columns: &[&str]) -> Self;
    fn with_table(self, alias: &str, table: &str, pk_columns: &[&str]) -> Self;
    fn with_fact(self, name: &str, table: &str, expr: &str, derived: &[(&str, &str)]) -> Self;
}

impl TestFixtureExt for SemanticViewDefinition {
    fn with_dimension(mut self, name: &str, expr: &str, source_table: Option<&str>) -> Self {
        self.dimensions.push(Dimension {
            name: name.to_string(),
            expr: expr.to_string(),
            source_table: source_table.map(|s| s.to_string()),
            output_type: None,
        });
        self
    }
    // ... etc
}
```
  </action>
  <verify>
    <automated>cd /Users/paul/Documents/Dev/Personal/duckdb-semantic-views && cargo test --lib model 2>&1 | tail -5</automated>
  </verify>
  <acceptance_criteria>
    - src/model.rs contains `pub fn inferred_types(&self) -> impl Iterator<Item = (&str, u32)>`
    - src/model.rs does NOT contain any changes to `column_type_names` or `column_types_inferred` field names or serde attributes
    - src/expand/test_helpers.rs contains `pub(super) fn orders_view() -> SemanticViewDefinition`
    - src/expand/test_helpers.rs contains at least one multi-table fixture function (e.g., `orders_with_customers`)
    - src/expand/test_helpers.rs is more than 50 lines (not a placeholder)
    - `cargo test --lib model` exits 0
  </acceptance_criteria>
  <done>inferred_types() method added to SemanticViewDefinition. expand/test_helpers.rs has shared fixture builders for single-table, multi-table, and mutation patterns.</done>
</task>

<task type="auto">
  <name>Task 2: Refactor sql_gen.rs tests to use shared fixtures</name>
  <files>src/expand/sql_gen.rs</files>
  <read_first>
    - src/expand/sql_gen.rs (full file -- 3,039 lines, read in sections: lines 1-219 for production code, lines 220-3039 for test code)
    - src/expand/test_helpers.rs (the fixtures just created in Task 1)
  </read_first>
  <action>
Refactor the test module in sql_gen.rs (lines 220-3039) to use the shared fixtures from test_helpers.rs instead of inline struct literals.

**Approach:**

1. At the top of the test module (`mod tests`), add:
   ```rust
   use super::test_helpers::{orders_view, orders_with_customers, TestFixtureExt};
   ```
   (Adjust imports based on what was created in Task 1.)

2. **Remove the inline `orders_view()` function** (lines 228-274) from the test module -- it is now in test_helpers.rs.

3. **For each test that builds a full SemanticViewDefinition inline**, replace with the appropriate shared fixture:

   - Tests that use a simple single-table orders view: replace ~25-line struct literal with `orders_view()`
   - Tests that need a variation (extra dimension, different metric): use `orders_view().with_dimension(...)` or `orders_view().with_metric(...)`
   - Tests that use a two-table orders+customers setup: replace with `orders_with_customers()`
   - Tests that need more complex setups: use the appropriate fixture + mutations

4. **Preserve test logic exactly** -- only the fixture construction changes. Do NOT modify any `expand()` call, `QueryRequest` construction, or assertion.

5. **Do NOT combine or remove tests** -- each test must remain as a separate `#[test]` function with the same name.

6. **Work through the test module systematically** from top to bottom. The test submodules are:
   - `expand_tests` (basic expansion, single/multi table)
   - Additional test modules for specific features (facts, derived metrics, fan traps, role playing, USING)

For tests where the fixture doesn't match any shared builder (highly specialized setups), leave the inline struct literal as-is. The goal is to replace the COMMON patterns, not achieve 100% extraction.

**Expected outcome:** The test section should shrink from ~1,800 lines to ~600-900 lines. Each refactored test should be 5-15 lines (fixture call + request + expand + assert) instead of 35-50 lines.
  </action>
  <verify>
    <automated>cd /Users/paul/Documents/Dev/Personal/duckdb-semantic-views && cargo test expand 2>&1 | tail -10</automated>
  </verify>
  <acceptance_criteria>
    - src/expand/sql_gen.rs test module contains `use super::test_helpers::` import(s)
    - src/expand/sql_gen.rs does NOT contain a local `fn orders_view()` definition in the test module (moved to test_helpers)
    - At least 20 tests in sql_gen.rs use a shared fixture function (grep for `orders_view()` or `orders_with_customers()` in test code)
    - src/expand/sql_gen.rs total line count is less than 2,200 lines (reduced from 3,039)
    - `cargo test expand` exits 0 with all 77 existing expand tests passing
    - No test function was removed or renamed
  </acceptance_criteria>
  <done>sql_gen.rs tests refactored to use shared fixtures. Test count unchanged. File size reduced by 800+ lines.</done>
</task>

</tasks>

<verification>
After both tasks complete:
1. `cargo test` passes (all Rust unit tests including all 77 expand tests)
2. `cargo test --lib model` passes (inferred_types method compiles correctly)
3. Total test count unchanged (no tests removed)
4. sql_gen.rs file size reduced from 3,039 lines
</verification>

<success_criteria>
- inferred_types() helper on SemanticViewDefinition expresses the parallel Vec invariant (per TIDY-03)
- sql_gen.rs test fixtures extracted to expand/test_helpers.rs with shared builders (per TIDY-05)
- All 77 expand tests pass with the refactored fixtures
- No serde format changes (backward compatibility preserved)
</success_criteria>

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