Support modern Rill metrics-view fields in import adapter#211
Support modern Rill metrics-view fields in import adapter#211nicosuave wants to merge 8 commits into
Conversation
Align the Rill adapter with the upstream Rill metrics-view parser: - Derive dimension/measure names when name is absent (column -> dimension_<i>, measure_<i>) instead of dropping them - Recognize the deprecated property: dimension shorthand alias for column: - Read the deprecated label: alias for display_name: on dimensions and measures - Support time_comparison measure type (mapped to derived, original type kept in metadata) - Support parent/derived metrics views (parent, parent_dimensions, parent_measures) via model metadata - Gracefully handle measure per:, lookup_table dimensions, and the rollups/ai_instructions/cache/watermark sections - Honor ignore: true on dimensions and measures
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0a93a481a3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
A Rill derived metrics view (parent + parent_dimensions/parent_measures with no own model/table) produced a Model with no table/sql/dax/source_uri, which validate_model rejects. That broke the CLI import path (sidemantic validate -> load_from_directory -> add_model) for any Rill project containing a derived view. Fall back to the parent metrics-view name as the model table so the imported model is a valid, queryable representation; meta still records the parent linkage. Update the non-execution fixture contract to expect the derived view as a source fragment without own fields.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 84230164a3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
A parent-only derived view now inherits its selected dimensions/measures (and the parent's real data source) when the parent metrics view is parsed in the same project. Previously the derived model was a valid but empty table, so fields like derived_view.revenue could not be resolved for CLI info/query. When the parent is absent (single-file parse) the view keeps the parent-name fallback table and remains valid.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d693f39317
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…ime defaults - Normalize parent_dimensions/parent_measures selector forms: '*' and omitted select all, lists select by name, and exclude/regex mappings are honored. Previously a '*' selector was iterated as characters, dropping all fields. - Map Rill time_comparison measures to Sidemantic's native time_comparison (base_metric + prior_period) so they query as real period-over-period comparisons instead of silently resolving to the current-period value. - Inherit the parent's default time dimension/grain (and materialize the referenced time dimension) on derived views that omit their own timeseries.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 47aff5984e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…ld grain
- When a derived view selects a derived/ratio parent measure, also inherit the
parent measures it references as hidden (public=False) fields so formulas like
'revenue / orders' resolve on the child.
- Support DuckDB-style 'expr' / string star selectors ('* EXCLUDE (...)') in
parent_dimensions/parent_measures instead of falling through to select-all.
- Preserve a child-only smallest_time_grain override when inheriting the
parent's timeseries, rather than overwriting it with the parent grain.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a32ee1279f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Resolve test_fixture_functionality_contracts.py: re-apply the three rill contract changes onto main's reorganized sets -- derived_metrics.yaml belongs in EXPECTED_LOW_SIGNAL_FIXTURES (one field-less model, not an empty graph), and RillAdapter joins source_fragments_without_fields and semantic_only_no_sources.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f61e9aaf48
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
A Rill measure with per: or requires: was unconditionally promoted to a derived metric. When the expression is an ordinary aggregation (e.g. SUM(amount)), promoting it to derived leaves the raw aggregate as the outer formula while the CTE only projects the decomposed column. That generates invalid SQL whenever a source column name also exists as a measure name (Binder error referencing the raw column). Only promote per/requires measures to derived when the expression is not itself a single top-level aggregation; otherwise keep simple aggregate parsing so it decomposes to agg/sql and stays queryable. The per/requires linkage is still preserved in metadata.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 88b86c231a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
A Rill derived metrics view (parent: ...) may only select inherited parent fields via parent_dimensions/parent_measures; Rill's own validation rejects a derived view that defines its own dimensions or measures. The importer now fails loudly in that case instead of parsing the child fields and exposing non-existent columns against the parent table.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6688eb567c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
1 participant
What changed
Brings the Rill import adapter (
sidemantic/adapters/rill.py) in line with the current upstream Rill metrics-view parser (rilldata/rillruntime/parser/parse_metrics_view.go). Previously the adapter silently dropped any dimension or measure that lacked an explicitname, ignored several deprecated aliases, and did not understand newer metrics-view sections.Upstream features now supported
namenow derive it fromcolumn(or fall back todimension_<i>); measures without anamefall back tomeasure_<i>. This matches Rill's own backwards-compatibility logic instead of dropping the field.property:shorthand. Recognized as the deprecated alias forcolumn:on dimensions.label:alias. Read as the deprecated alias fordisplay_name:on dimensions and measures (previously onlydisplay_namewas honored).time_comparisonmeasure type. Parsed into a derived calculation with the original Rill type preserved in metadata.parent,parent_dimensions, andparent_measuresare captured in model metadata so the inheritance linkage survives import.per:,lookup_tabledimensions, and therollups,ai_instructions,cache, andwatermarksections no longer interfere with parsing;lookup_tabledimensions are preserved with their lookup config in metadata rather than dropped.ignore: trueon a dimension or measure is honored.Backward compatibility is preserved: all formats and fields the adapter already handled continue to parse identically, and existing Rill fixtures/tests stay green.
Tests and fixtures
tests/fixtures/rill/parent_metrics.yamlandtests/fixtures/rill/derived_metrics.yaml, with field names verified against the upstream Rill parser.tests/adapters/rill/test_modern_features.pycovering aliases, name derivation,time_comparison,per, lookup dimensions, and parent/derived views.Known limitations
time_comparisonmeasures are mapped to Sidemanticderivedmetrics (with the original type recorded in metadata), since Rill does not express the explicit base-metric/comparison wiring that Sidemantic's nativetime_comparisontype requires.