---
phase: 50-code-quality-test-coverage
plan: 02
type: execute
wave: 2
depends_on: [50-01]
files_modified:
  - src/expand/sql_gen.rs
  - src/expand/types.rs
  - src/expand/semi_additive.rs
  - src/model.rs
  - src/table_function.rs
autonomous: true
requirements: [QUAL-02, QUAL-03, QUAL-04, QUAL-05]
must_haves:
  truths:
    - "Dimension and metric resolution loops in expand() and expand_facts() use a shared generic helper instead of duplicated code"
    - "DimensionName and MetricName newtypes exist with case-insensitive Eq and Hash"
    - "QueryRequest uses Vec<DimensionName> and Vec<MetricName> instead of Vec<String>"
    - "NaGroup named struct replaces tuple type in semi_additive.rs"
    - "parse_constraint_columns function and its tests are removed from model.rs"
  artifacts:
    - path: "src/expand/sql_gen.rs"
      provides: "resolve_names generic helper function and callers using it"
      contains: "fn resolve_names"
    - path: "src/expand/types.rs"
      provides: "DimensionName and MetricName newtypes, updated QueryRequest"
      contains: "pub struct DimensionName"
    - path: "src/expand/semi_additive.rs"
      provides: "NaGroup named struct"
      contains: "struct NaGroup"
    - path: "src/model.rs"
      provides: "parse_constraint_columns removed"
  key_links:
    - from: "src/expand/types.rs DimensionName"
      to: "src/expand/sql_gen.rs resolve_names"
      via: "resolution helper accepts newtypes"
      pattern: "DimensionName"
    - from: "src/expand/types.rs QueryRequest"
      to: "src/table_function.rs"
      via: "VTab builds QueryRequest with newtypes"
      pattern: "DimensionName::new"
---

<objective>
Refactor expand module internals: deduplicate resolution loops into a shared generic helper, introduce DimensionName/MetricName newtypes with case-insensitive semantics, replace the tuple type in semi_additive.rs with a named struct, and remove dead code from model.rs.

Purpose: Reduce code duplication, improve type safety via domain newtypes, and clean up dead code. Plan 01's tests serve as the regression safety net.
Output: Cleaner code with ~60 fewer lines of duplicated resolution logic, compile-time type safety for dimension/metric name handling, and zero dead code annotations.
</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/50-code-quality-test-coverage/50-RESEARCH.md
@.planning/phases/50-code-quality-test-coverage/50-01-SUMMARY.md

@src/expand/mod.rs
@src/expand/test_helpers.rs

<interfaces>
<!-- Current state that will be refactored. Executor must read these files. -->

From src/expand/types.rs (BEFORE refactoring):
```rust
pub struct QueryRequest {
    pub dimensions: Vec<String>,
    pub metrics: Vec<String>,
    pub facts: Vec<String>,
}
```

From src/expand/sql_gen.rs expand() lines 242-295 (dim resolution loop):
```rust
let mut resolved_dims = Vec::with_capacity(req.dimensions.len());
let mut seen_dims = std::collections::HashSet::new();
for name in &req.dimensions {
    if !seen_dims.insert(name.to_ascii_lowercase()) {
        return Err(ExpandError::DuplicateDimension { ... });
    }
    let dim = find_dimension(def, name).ok_or_else(|| { ... })?;
    resolved_dims.push(dim);
}
// ... identical pattern for metrics with PrivateMetric check
```

From src/expand/sql_gen.rs expand_facts() lines 26-80 (fact + dim resolution):
```rust
// Same pattern for facts with PrivateFact check
// Same pattern for dims (duplicated from expand())
```

From src/expand/semi_additive.rs collect_na_groups() line 315-316:
```rust
fn collect_na_groups(...) -> Vec<(Vec<NonAdditiveDim>, Vec<usize>)> {
    let mut groups: Vec<(Vec<String>, Vec<NonAdditiveDim>, Vec<usize>)> = Vec::new();
```

From src/model.rs line 420-434:
```rust
#[allow(dead_code)]
pub(crate) fn parse_constraint_columns(s: &str) -> Vec<String> { ... }
```
</interfaces>
</context>

<tasks>

<task type="auto">
  <name>Task 1: Extract resolve_names generic helper and deduplicate resolution loops</name>
  <files>src/expand/sql_gen.rs</files>
  <read_first>
    src/expand/sql_gen.rs
    src/expand/types.rs
    src/expand/resolution.rs
    src/expand/test_helpers.rs
  </read_first>
  <action>
Add a generic `resolve_names` function at the top of `sql_gen.rs` (above `expand_facts`) that replaces the 4 duplicated resolution loops (dims in expand, metrics in expand, facts in expand_facts, dims in expand_facts).

Define the function:
```rust
/// Resolve a list of names against a definition, checking for duplicates,
/// unknown names, and optional access restrictions.
///
/// Generic helper that deduplicates the resolution pattern used for
/// dimensions, metrics, and facts.
fn resolve_names<'a, T>(
    names: &[String],
    view_name: &str,
    find_fn: impl Fn(&str) -> Option<&'a T>,
    is_private: impl Fn(&T) -> bool,
    available_fn: impl Fn() -> Vec<String>,
    suggest_fn: impl Fn(&str) -> Option<String>,
    make_dup_err: impl Fn(String, String) -> ExpandError,
    make_not_found_err: impl Fn(String, String, Vec<String>, Option<String>) -> ExpandError,
    make_private_err: impl Fn(String, String) -> ExpandError,
) -> Result<Vec<&'a T>, ExpandError> {
    let mut resolved = Vec::with_capacity(names.len());
    let mut seen = std::collections::HashSet::new();
    for name in names {
        if !seen.insert(name.to_ascii_lowercase()) {
            return Err(make_dup_err(view_name.to_string(), name.clone()));
        }
        let item = find_fn(name).ok_or_else(|| {
            let avail = available_fn();
            let suggestion = suggest_fn(name);
            make_not_found_err(view_name.to_string(), name.clone(), avail, suggestion)
        })?;
        if is_private(item) {
            return Err(make_private_err(view_name.to_string(), name.clone()));
        }
        resolved.push(item);
    }
    Ok(resolved)
}
```

Then replace the 4 call sites:

**In `expand()` -- dimension resolution (lines ~243-263):**
Replace with:
```rust
let resolved_dims = resolve_names(
    &req.dimensions,
    view_name,
    |name| find_dimension(def, name),
    |_dim| false, // dims have no private check
    || def.dimensions.iter().map(|d| d.name.clone()).collect(),
    |name| suggest_closest(name, &def.dimensions.iter().map(|d| d.name.clone()).collect::<Vec<_>>()),
    |vn, n| ExpandError::DuplicateDimension { view_name: vn, name: n },
    |vn, n, avail, sug| ExpandError::UnknownDimension { view_name: vn, name: n, available: avail, suggestion: sug },
    |vn, n| ExpandError::DuplicateDimension { view_name: vn, name: n }, // unused, dims aren't private
)?;
```

**In `expand()` -- metric resolution (lines ~265-295):**
Replace with:
```rust
let resolved_mets = resolve_names(
    &req.metrics,
    view_name,
    |name| find_metric(def, name),
    |met| met.access == AccessModifier::Private,
    || def.metrics.iter().map(|m| m.name.clone()).collect(),
    |name| suggest_closest(name, &def.metrics.iter().map(|m| m.name.clone()).collect::<Vec<_>>()),
    |vn, n| ExpandError::DuplicateMetric { view_name: vn, name: n },
    |vn, n, avail, sug| ExpandError::UnknownMetric { view_name: vn, name: n, available: avail, suggestion: sug },
    |vn, n| ExpandError::PrivateMetric { view_name: vn, name: n },
)?;
```

**In `expand_facts()` -- fact resolution (lines ~26-57):**
Replace with:
```rust
let resolved_facts = resolve_names(
    &req.facts,
    view_name,
    |name| def.facts.iter().find(|f| f.name.eq_ignore_ascii_case(name)),
    |fact| fact.access == AccessModifier::Private,
    || def.facts.iter().map(|f| f.name.clone()).collect(),
    |name| suggest_closest(name, &def.facts.iter().map(|f| f.name.clone()).collect::<Vec<_>>()),
    |vn, n| ExpandError::DuplicateFact { view_name: vn, name: n },
    |vn, n, avail, sug| ExpandError::UnknownFact { view_name: vn, name: n, available: avail, suggestion: sug },
    |vn, n| ExpandError::PrivateFact { view_name: vn, name: n },
)?;
```

**In `expand_facts()` -- dimension resolution (lines ~60-80):**
Replace with same pattern as expand()'s dimension resolution.

IMPORTANT: The behavior must be IDENTICAL to current code. The same error variants must be returned for the same inputs. Run the full test suite after this change to verify no regression.

After refactoring, verify the `#[allow(clippy::too_many_lines)]` annotations on `expand` and `expand_facts` can be removed if the functions are now short enough (under the clippy threshold of ~100 lines). If they're still over the threshold, keep the annotation.
  </action>
  <verify>
    <automated>cd /Users/paul/Documents/Dev/Personal/duckdb-semantic-views && cargo test --lib 2>&1 | tail -5</automated>
  </verify>
  <acceptance_criteria>
    - sql_gen.rs contains `fn resolve_names<'a, T>(` function definition
    - sql_gen.rs expand() function calls `resolve_names(` for both dimensions and metrics (no manual HashSet loop)
    - sql_gen.rs expand_facts() function calls `resolve_names(` for both facts and dimensions (no manual HashSet loop)
    - sql_gen.rs does NOT contain 4 separate `let mut seen_` variables (deduplication achieved)
    - `cargo test --lib` exits 0 with all tests passing (no behavioral changes)
  </acceptance_criteria>
  <done>Resolution loops in expand() and expand_facts() are deduplicated into resolve_names generic helper. All 4 call sites use the helper. All existing tests pass unchanged.</done>
</task>

<task type="auto">
  <name>Task 2: Introduce DimensionName and MetricName newtypes in QueryRequest</name>
  <files>src/expand/types.rs, src/expand/sql_gen.rs, src/expand/test_helpers.rs, src/table_function.rs</files>
  <read_first>
    src/expand/types.rs
    src/expand/sql_gen.rs
    src/expand/test_helpers.rs
    src/table_function.rs
    src/expand/mod.rs
  </read_first>
  <action>
**Step 1: Define newtypes in `src/expand/types.rs`.**

Add these type definitions ABOVE the `QueryRequest` struct:

```rust
/// A dimension name with case-insensitive equality and hashing.
///
/// Wraps a `String` and provides `PartialEq`/`Eq`/`Hash` based on ASCII-lowercased form.
/// This centralizes the 189+ ad-hoc `eq_ignore_ascii_case` / `to_ascii_lowercase` calls
/// throughout the resolution code.
#[derive(Debug, Clone)]
pub struct DimensionName(String);

impl DimensionName {
    pub fn new(s: impl Into<String>) -> Self {
        Self(s.into())
    }

    pub fn as_str(&self) -> &str {
        &self.0
    }
}

impl PartialEq for DimensionName {
    fn eq(&self, other: &Self) -> bool {
        self.0.eq_ignore_ascii_case(&other.0)
    }
}

impl Eq for DimensionName {}

impl std::hash::Hash for DimensionName {
    fn hash<H: std::hash::Hasher>(&self, state: &mut H) {
        for byte in self.0.bytes() {
            byte.to_ascii_lowercase().hash(state);
        }
    }
}

impl std::fmt::Display for DimensionName {
    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
        write!(f, "{}", self.0)
    }
}

impl std::ops::Deref for DimensionName {
    type Target = str;
    fn deref(&self) -> &str {
        &self.0
    }
}

impl From<String> for DimensionName {
    fn from(s: String) -> Self {
        Self(s)
    }
}

impl From<&str> for DimensionName {
    fn from(s: &str) -> Self {
        Self(s.to_string())
    }
}
```

Then define `MetricName` with the EXACT same pattern (different struct name). Copy the entire block, replacing `DimensionName` with `MetricName`.

**Step 2: Update `QueryRequest` in `src/expand/types.rs`:**

Change:
```rust
pub struct QueryRequest {
    pub dimensions: Vec<String>,
    pub metrics: Vec<String>,
    pub facts: Vec<String>,
}
```
to:
```rust
pub struct QueryRequest {
    pub dimensions: Vec<DimensionName>,
    pub metrics: Vec<MetricName>,
    pub facts: Vec<String>,  // facts stays as String per scope
}
```

**Step 3: Update `src/expand/sql_gen.rs` resolve_names calls.**

The `resolve_names` function currently accepts `&[String]`. Change its first parameter to accept names that can be dereferenced to `&str`. The simplest approach: change the loop to use `.as_ref()` or `Deref`. Since both `DimensionName` and `MetricName` implement `Deref<Target=str>`, update the function signature:

```rust
fn resolve_names<'a, T, N: AsRef<str>>(
    names: &[N],
    ...
```

And change the loop body:
```rust
for name in names {
    let name_str = name.as_ref();
    if !seen.insert(name_str.to_ascii_lowercase()) {
        return Err(make_dup_err(view_name.to_string(), name_str.to_string()));
    }
    let item = find_fn(name_str).ok_or_else(|| {
        ...
        make_not_found_err(view_name.to_string(), name_str.to_string(), avail, suggestion)
    })?;
    if is_private(item) {
        return Err(make_private_err(view_name.to_string(), name_str.to_string()));
    }
    resolved.push(item);
}
```

**Step 4: Update `src/expand/mod.rs` exports.**

Add to the public exports:
```rust
pub use types::{DimensionName, MetricName};
```

**Step 5: Update `src/table_function.rs`.**

Find where `QueryRequest` is constructed (it reads VARCHAR parameters from the VTab and builds the struct). Change from:
```rust
dimensions: dim_names,  // Vec<String>
metrics: met_names,     // Vec<String>
```
to:
```rust
dimensions: dim_names.into_iter().map(DimensionName::new).collect(),
metrics: met_names.into_iter().map(MetricName::new).collect(),
```

Add `use crate::expand::{DimensionName, MetricName};` at the top of `table_function.rs`.

**Step 6: Update test code in `src/expand/sql_gen.rs`.**

All test functions that construct `QueryRequest` need updating. Since there are ~95 tests, the most efficient approach is a bulk find-and-replace:

- Replace `dimensions: vec!["foo".to_string()` with `dimensions: vec![DimensionName::new("foo")`
- Replace `metrics: vec!["bar".to_string()` with `metrics: vec![MetricName::new("bar")`
- Add `use crate::expand::types::{DimensionName, MetricName};` to the test module imports

For `.to_string()` chains in dimensions/metrics vecs, the pattern is:
- `"name".to_string()` becomes `DimensionName::new("name")` or `MetricName::new("name")`

**Step 7: Add newtype unit tests in `src/expand/types.rs`.**

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

    #[test]
    fn dimension_name_case_insensitive_eq() {
        assert_eq!(DimensionName::new("Foo"), DimensionName::new("foo"));
        assert_eq!(DimensionName::new("FOO"), DimensionName::new("foo"));
        assert_ne!(DimensionName::new("foo"), DimensionName::new("bar"));
    }

    #[test]
    fn dimension_name_case_insensitive_hash() {
        use std::collections::HashSet;
        let mut set = HashSet::new();
        set.insert(DimensionName::new("Foo"));
        assert!(set.contains(&DimensionName::new("foo")));
        assert!(set.contains(&DimensionName::new("FOO")));
        assert!(!set.contains(&DimensionName::new("bar")));
    }

    #[test]
    fn metric_name_case_insensitive_eq() {
        assert_eq!(MetricName::new("Revenue"), MetricName::new("revenue"));
        assert_ne!(MetricName::new("revenue"), MetricName::new("cost"));
    }

    #[test]
    fn metric_name_case_insensitive_hash() {
        use std::collections::HashSet;
        let mut set = HashSet::new();
        set.insert(MetricName::new("Revenue"));
        assert!(set.contains(&MetricName::new("revenue")));
        assert!(!set.contains(&MetricName::new("cost")));
    }

    #[test]
    fn dimension_name_display() {
        let name = DimensionName::new("Region");
        assert_eq!(format!("{name}"), "Region");
    }

    #[test]
    fn metric_name_deref_to_str() {
        let name = MetricName::new("total_revenue");
        let s: &str = &name;
        assert_eq!(s, "total_revenue");
    }

    #[test]
    fn dimension_name_from_string() {
        let name: DimensionName = "foo".into();
        assert_eq!(name.as_str(), "foo");
        let name2: DimensionName = String::from("bar").into();
        assert_eq!(name2.as_str(), "bar");
    }
}
```

IMPORTANT: After all changes, run `cargo test --lib` to verify all tests pass. The newtypes must be drop-in compatible -- the Deref impl lets existing string comparisons work. If any proptest files construct QueryRequest, update those too.
  </action>
  <verify>
    <automated>cd /Users/paul/Documents/Dev/Personal/duckdb-semantic-views && cargo test --lib 2>&1 | tail -5</automated>
  </verify>
  <acceptance_criteria>
    - types.rs contains `pub struct DimensionName(String)` and `pub struct MetricName(String)`
    - types.rs DimensionName has `impl PartialEq` with `eq_ignore_ascii_case`
    - types.rs DimensionName has `impl Hash` with `to_ascii_lowercase`
    - types.rs MetricName has `impl PartialEq` with `eq_ignore_ascii_case`
    - types.rs QueryRequest has `pub dimensions: Vec<DimensionName>` and `pub metrics: Vec<MetricName>`
    - types.rs contains `#[cfg(test)] mod tests` with at least 7 tests for newtype behavior
    - mod.rs contains `pub use types::{DimensionName, MetricName}`
    - table_function.rs contains `DimensionName::new(` or `DimensionName::from(`
    - `cargo test --lib` exits 0 with all tests passing
  </acceptance_criteria>
  <done>DimensionName and MetricName newtypes defined with case-insensitive Eq/Hash. QueryRequest updated to use newtypes. All callers (sql_gen tests, table_function, proptests) updated. 7+ newtype unit tests added and passing.</done>
</task>

<task type="auto">
  <name>Task 3: NaGroup named struct and dead code removal</name>
  <files>src/expand/semi_additive.rs, src/model.rs</files>
  <read_first>
    src/expand/semi_additive.rs
    src/model.rs
  </read_first>
  <action>
**Part A: Replace tuple type with NaGroup struct in semi_additive.rs.**

At the top of `semi_additive.rs` (after the imports), add:

```rust
/// A group of metrics sharing the same NON ADDITIVE BY dimension set.
///
/// Replaces the tuple `(Vec<NonAdditiveDim>, Vec<usize>)` with named fields
/// for readability.
struct NaGroup {
    /// The actual NonAdditiveDim entries for this group.
    na_dims: Vec<NonAdditiveDim>,
    /// Indices into `resolved_mets` that belong to this group.
    metric_indices: Vec<usize>,
}
```

Then update `collect_na_groups`:

1. Change return type from `Vec<(Vec<NonAdditiveDim>, Vec<usize>)>` to `Vec<NaGroup>`.

2. Change the internal `groups` variable from `Vec<(Vec<String>, Vec<NonAdditiveDim>, Vec<usize>)>` to a new internal type. The cleanest approach is a temporary struct or keep a Vec of tuples internally but output NaGroup:

```rust
fn collect_na_groups(
    resolved_mets: &[&Metric],
    queried_dim_names: &HashSet<String>,
) -> Vec<NaGroup> {
    // Internal representation for grouping by key
    let mut groups: Vec<(Vec<String>, Vec<NonAdditiveDim>, Vec<usize>)> = Vec::new();
    // ... existing grouping logic unchanged ...
    groups
        .into_iter()
        .map(|(_, na_dims, metric_indices)| NaGroup { na_dims, metric_indices })
        .collect()
}
```

3. Update all consumers. Search for `.0` and `.1` tuple access patterns on the return value of `collect_na_groups`. The consumers are in `expand_semi_additive` function. Replace:
   - `(na_dims, _met_indices)` with `NaGroup { na_dims, metric_indices: _ }` or `group.na_dims` / `group.metric_indices`
   - `(na_dims, met_indices)` with `NaGroup { na_dims, metric_indices }` or field access

Specifically, find these patterns and update:
   - Line ~117: `for (group_idx, (na_dims, _met_indices)) in na_groups.iter().enumerate()` -> `for (group_idx, group) in na_groups.iter().enumerate()` then use `group.na_dims`
   - Any other destructuring of the tuple pair -> use NaGroup field access

4. Also check `get_rn_column_for_metric` if it accesses the tuple -- update similarly.

**Part B: Remove dead `parse_constraint_columns` from model.rs.**

1. Remove the function `parse_constraint_columns` at lines 420-434 (the `#[allow(dead_code)]` annotated function).

2. Remove the entire `mod constraint_column_parsing_tests` block at lines 510-542 (the 5 tests for the removed function).

3. Verify no other code references `parse_constraint_columns` by confirming `cargo test --lib` and `cargo test --features extension --lib` (if applicable) both compile successfully.

Run `cargo test --lib` after each part to catch regressions incrementally.
  </action>
  <verify>
    <automated>cd /Users/paul/Documents/Dev/Personal/duckdb-semantic-views && cargo test expand::semi_additive --lib 2>&1 | tail -5 && cargo test model --lib 2>&1 | tail -5</automated>
  </verify>
  <acceptance_criteria>
    - semi_additive.rs contains `struct NaGroup` with fields `na_dims: Vec<NonAdditiveDim>` and `metric_indices: Vec<usize>`
    - semi_additive.rs `collect_na_groups` return type is `Vec<NaGroup>` (not `Vec<(Vec<NonAdditiveDim>, Vec<usize>)>`)
    - semi_additive.rs does NOT contain `.0` or `.1` tuple access on NaGroup values
    - model.rs does NOT contain `parse_constraint_columns`
    - model.rs does NOT contain `#[allow(dead_code)]` on any function (the only one was parse_constraint_columns)
    - model.rs does NOT contain `constraint_column_parsing_tests`
    - `cargo test expand::semi_additive --lib` exits 0 (all 10 existing tests pass)
    - `cargo test model --lib` exits 0 (removed tests no longer counted, remaining tests pass)
  </acceptance_criteria>
  <done>NaGroup named struct replaces tuple type in semi_additive.rs. Dead parse_constraint_columns function and its 5 tests removed from model.rs. All existing tests pass.</done>
</task>

</tasks>

<threat_model>
## Trust Boundaries

No new trust boundaries introduced. This plan refactors internal code structure without changing parsing, FFI, or persistence logic. QueryRequest construction in table_function.rs uses the same input values -- only the container types change.

## STRIDE Threat Register

| Threat ID | Category | Component | Disposition | Mitigation Plan |
|-----------|----------|-----------|-------------|-----------------|
| T-50-02 | T (Tampering) | resolve_names helper | accept | Behavioral equivalence verified by Plan 01's regression tests + existing 576+ unit tests |
| T-50-03 | T (Tampering) | Newtype Eq/Hash consistency | mitigate | Unit tests verify DimensionName("Foo") == DimensionName("foo") and matching Hash behavior; inconsistency would break HashSet dedup |
</threat_model>

<verification>
```bash
# Per-task verification
cargo test --lib  # after each task

# Full suite (wave gate)
just test-all
```
</verification>

<success_criteria>
1. `cargo test --lib` passes with all tests (576+)
2. sql_gen.rs contains `fn resolve_names` and expand()/expand_facts() use it
3. types.rs contains DimensionName and MetricName with case-insensitive Eq/Hash
4. QueryRequest uses Vec<DimensionName> and Vec<MetricName>
5. semi_additive.rs contains NaGroup struct, no tuple access patterns
6. model.rs has no `parse_constraint_columns` or `#[allow(dead_code)]`
7. `just test-all` passes
</success_criteria>

<output>
After completion, create `.planning/phases/50-code-quality-test-coverage/50-02-SUMMARY.md`
</output>
