Skip to content

refactor: unify facet filter parsing across CLI commands#613

Merged
lewisjared merged 2 commits intomainfrom
fix/unify-facet-filter-parsing
Apr 10, 2026
Merged

refactor: unify facet filter parsing across CLI commands#613
lewisjared merged 2 commits intomainfrom
fix/unify-facet-filter-parsing

Conversation

@lewisjared
Copy link
Copy Markdown
Contributor

Description

Consolidate three inconsistent filter parsing implementations into a single parse_facet_filters returning dict[str, list[str]] with multi-value OR semantics.

Problem: The CLI had three different filter parsing paths with different behavior:

  • cli/_utils.py:parse_facet_filters returned dict[str, str] — duplicate keys silently overwrote with a warning
  • cli/datasets.py had inline parsing returning dict[str, list[str]] — multi-value OR support
  • cli/solve.py duplicated the same inline parsing from datasets.py

This meant ref executions list-groups --filter source_id=A --filter source_id=B only matched B (overwrite), while ref datasets list --dataset-filter source_id=A --dataset-filter source_id=B correctly matched A or B.

Solution:

  • parse_facet_filters now returns dict[str, list[str]] with multi-value OR semantics
  • All CLI commands (datasets list, solve, executions list-groups/delete-groups) use the shared function
  • _filter_executions_by_facets and get_execution_group_and_latest_filtered updated to accept multi-value filters
  • Extracted _selectors_match_facet helper to keep complexity under ruff limits
  • Fixed wrong docstring on datasets list-columns (was copy-pasted from config command)

Checklist

Please confirm that this pull request has done the following:

  • Tests added
  • Documentation added (where applicable)
  • Changelog item added to changelog/

Consolidate three inconsistent filter parsing implementations into a
single `parse_facet_filters` returning `dict[str, list[str]]` with
multi-value OR semantics. Previously, `--filter key=val key=val2` in
executions silently overwrote the first value; it now correctly returns
results matching either value, consistent with `--dataset-filter` in
`datasets list` and `solve`.

- `cli/_utils.py`: `parse_facet_filters` returns multi-value dict
- `cli/datasets.py`, `cli/solve.py`: replace inline parsing with shared function
- `cli/executions.py`: update `ListGroupsFilterOptions.facets` type
- `models/execution.py`: update `_filter_executions_by_facets` signature,
  extract `_selectors_match_facet` helper
- Fix wrong docstring on `datasets list-columns`
@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 5, 2026

Codecov Report

❌ Patch coverage is 92.59259% with 2 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
...es/climate-ref/src/climate_ref/models/execution.py 81.81% 1 Missing and 1 partial ⚠️
Flag Coverage Δ
core 93.29% <92.59%> (-0.04%) ⬇️
providers 91.85% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
packages/climate-ref/src/climate_ref/cli/_utils.py 95.91% <100.00%> (-0.17%) ⬇️
...ckages/climate-ref/src/climate_ref/cli/datasets.py 91.89% <100.00%> (-0.11%) ⬇️
...ages/climate-ref/src/climate_ref/cli/executions.py 94.88% <100.00%> (ø)
packages/climate-ref/src/climate_ref/cli/solve.py 100.00% <100.00%> (ø)
...es/climate-ref/src/climate_ref/models/execution.py 98.56% <81.81%> (-0.77%) ⬇️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR refactors the CLI facet/dataset filter handling to use a single shared parser with consistent “AND across keys, OR within key” semantics, eliminating divergent behavior across commands.

Changes:

  • Updated parse_facet_filters to return dict[str, list[str]] and collect repeated keys as multi-value OR filters.
  • Switched datasets list, solve, and executions list-groups/delete-groups to use the shared parser; updated execution-group facet filtering to support multi-value filters.
  • Updated/added unit tests and added a changelog entry; corrected the datasets list-columns docstring.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.

Show a summary per file
File Description
packages/climate-ref/src/climate_ref/cli/_utils.py Changes parse_facet_filters to return multi-value lists and updates validation/errors.
packages/climate-ref/src/climate_ref/cli/datasets.py Replaces inline dataset-filter parsing with shared parse_facet_filters; fixes list_columns docstring.
packages/climate-ref/src/climate_ref/cli/solve.py Uses shared parse_facet_filters for --dataset-filter and maps parsing errors to BadParameter.
packages/climate-ref/src/climate_ref/cli/executions.py Updates facet filter typing and “no results” messaging for multi-value facet filters.
packages/climate-ref/src/climate_ref/models/execution.py Adds helper and updates facet filtering to support OR-within-key semantics.
packages/climate-ref/tests/unit/cli/test_utils.py Updates expectations and adds tests for multi-value and dotted keys.
packages/climate-ref/tests/unit/cli/test_solve.py Updates error-message assertions to match new parsing errors.
packages/climate-ref/tests/unit/cli/test_executions.py Updates tests to validate OR semantics for repeated facet keys in executions filtering.
changelog/613.improvement.md Documents the user-visible change to executions --filter behavior.
Comments suppressed due to low confidence (2)

packages/climate-ref/src/climate_ref/cli/_utils.py:80

  • The ValueError message here suggests dataset_type.key=value is a supported/expected format. That’s true for executions --filter, but solve --dataset-filter/datasets list --dataset-filter don’t support dataset-type scoping (those keys won’t match catalog columns and may be ignored or rejected later). Consider tightening this message to just key=value, or explicitly clarifying that dataset_type.key=value is only supported for execution facet filters so users aren’t misled.
        if "=" not in filter_str:
            raise ValueError(
                f"Invalid filter format: '{filter_str}'. "
                f"Expected format: 'key=value' or 'dataset_type.key=value' "
                f"(e.g., 'source_id=GFDL-ESM4' or 'cmip6.source_id=GFDL-ESM4')"
            )

packages/climate-ref/src/climate_ref/cli/solve.py:99

  • Now that --dataset-filter uses the shared parse_facet_filters, keys containing a dot (e.g. cmip6.source_id=...) will parse successfully. However apply_dataset_filters() only matches literal column names and skips unknown facets, so dotted keys will silently have no effect in solve. Consider validating parsed_dataset_filters here (e.g., reject keys containing '.') or normalizing them so dataset filters don’t become accidental no-ops.
    try:
        parsed_dataset_filters = parse_facet_filters(dataset_filter) or None
    except ValueError as e:
        raise typer.BadParameter(str(e), param_hint="--dataset-filter")

    filters = SolveFilterOptions(
        diagnostic=diagnostic,
        provider=provider,
        dataset=parsed_dataset_filters,
    )

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@lewisjared lewisjared merged commit 5f80c8f into main Apr 10, 2026
30 checks passed
@lewisjared lewisjared deleted the fix/unify-facet-filter-parsing branch April 10, 2026 14:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants