Greptile Summary

Tightens the _validate_subcategory_parents model validator in DataDesignerConfig so that a subcategory column's parent must be a sampler column whose sampler_type is specifically category, not just any sampler.

• The error message now differentiates between a parent that isn't a sampler at all vs. a sampler parent with the wrong sampler_type, giving callers a precise description of what was configured incorrectly.
• A new regression test covers the uniform-parent case and validates the updated error message text end-to-end.

Important Files Changed

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[_validate_subcategory_parents] --> B{column_type == 'sampler'\nAND sampler_type == SUBCATEGORY?}
    B -- No --> C[skip]
    B -- Yes --> D[lookup parent by col.params.category]
    D --> E{parent found?}
    E -- No --> F[defer to schema validator]
    E -- Yes --> G{parent.column_type == 'sampler'\nAND parent.sampler_type == CATEGORY?}
    G -- Yes --> H[valid ✓]
    G -- No --> I{parent.column_type == 'sampler'?}
    I -- Yes --> J[error: sampler column with wrong sampler_type]
    I -- No --> K[error: non-sampler column type]

_{Reviews (1): Last reviewed commit: "validate subcategory parent sampler type" | Re-trigger Greptile}

PR #628 Review — fix(config): validate subcategory parent sampler type

Summary

This PR tightens DataDesignerConfig._validate_subcategory_parents so that a
subcategory sampler's parent must be a category sampler — previously the
validator only rejected non-sampler parents, letting e.g. a uniform sampler
silently slip through even though the error message already promised
sampler_type='category'. The error message is updated to describe the actual
offending parent type, and a regression test is added for the
uniform-parent case.

Scope is small (+33/-3, two files), contained to data-designer-config, and
consistent with the module's existing validation style.

Findings

Correctness

• Logic is right. The guard condition
  parent.column_type != "sampler" or parent.sampler_type != SamplerType.CATEGORY
  correctly admits only category samplers, matching the error message's
  contract. The two-branch message split keeps the non-sampler diagnostic
  intact.
• getattr(parent.sampler_type, "value", parent.sampler_type) fallback is
  unneeded but harmless. Every concrete sampler params class in
  sampler_params.py declares sampler_type: Literal[SamplerType.X] = SamplerType.X,
  so on a validated model parent.sampler_type is always a SamplerType
  enum. You could simplify to parent.sampler_type.value. Not worth blocking
  on — the defensive form is cheap insurance if a future sampler class ever
  stores a raw string.
• Missing-parent path untouched. The parent is not None short-circuit
  still defers to the schema validator for unknown parent names
  (confirmed by test_subcategory_parent_missing_defers_to_schema_validator).
  Good.

Test coverage

• The new test_subcategory_parent_must_be_a_category_sampler exercises the
  newly-tightened path with a uniform parent and asserts the message
  mentions both the actual parent sampler type and the expected one via a
  regex — this is the right shape.
• The existing test_subcategory_parent_as_category_sampler_is_valid and
  the non-sampler-parent case (earlier in the file) together cover the three
  meaningful branches: valid category parent, non-sampler parent, wrong-type
  sampler parent. Coverage is complete for this validator.
• Minor nit: no assertion that the error message still identifies the parent
  column by name in the new case. The existing non-sampler test likely
  covers message shape; if not, consider adding "'package_type'" to the
  regex for symmetry.

Style & conventions

• Follows project conventions: from __future__ import annotations already
  present, absolute imports, modern typing, Pydantic @model_validator
  pattern matching the rest of the file.
• Line 57's combined condition is getting slightly long/dense. A short
  local would read more cleanly, e.g.:

  is_category_sampler = parent.column_type == "sampler" and parent.sampler_type == SamplerType.CATEGORY
  if parent is not None and not is_category_sampler:
      ...

  Purely stylistic — current form is fine.

Performance / security / backward compatibility

• No perf impact (single-pass validation over columns, unchanged shape).
• No security implications.
• Behavior change worth flagging in release notes. Configs that were
  previously accepted — a subcategory column pointing at a non-category
  sampler parent — will now raise at model_validate time. This is the
  point of the fix, but users with such configs (if any) will see a new
  ValueError on upgrade. Since the downstream generation almost certainly
  failed anyway (subcategory expects categorical parent values), the
  practical blast radius is small.
• Architecture docs are correctly marked N/A — this is a validation-only
  tightening inside a single module, no import-direction or structural
  changes.

Verdict

Looks good. Small, focused, correct, tested. Non-blocking suggestions:

1. Drop the getattr(..., "value", ...) fallback in favor of
   parent.sampler_type.value (the field is always a SamplerType enum).
2. Consider a one-line release note mentioning the now-rejected configs
   since this changes validation outcomes.
3. Optional: extract the category-check into a named local for readability.

None of these are required for merge.