Keep unnamed Rill timeseries expression dimensions addressable#227
Keep unnamed Rill timeseries expression dimensions addressable#227nicosuave wants to merge 2 commits into
Conversation
When a Rill metrics view declares timeseries: order_date alongside an
unnamed expression dimension whose expression is that same column
({expression: order_date}), the name fallback produced dimension_0. The
auto-create check then saw the column already present and skipped
creating an order_date dimension, while default_time_dimension stayed
order_date, so validate_model rejected the model because the default
time dimension referenced no dimension.
Name such an unnamed dimension after the timeseries column so the
generated time dimension stays addressable and default_time_dimension
resolves.
![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: 5494325ff1
ℹ️ 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".
The unnamed-expression-dimension fallback renamed every dimension whose expression equalled the timeseries column to the timeseries name. Two such dimensions both became the timeseries column name, so validate_model rejected the duplicate dimension names and add_model failed on an otherwise parseable Rill project. Only the first unnamed match now claims the timeseries name; later matches keep their positional dimension_<i> name, matching Rill's own fallback.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: efbf83298c
ℹ️ 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".
| if timeseries_column and sql_expr == timeseries_column and not timeseries_name_taken: | ||
| name = timeseries_column |
There was a problem hiding this comment.
When an expression-only timeseries dimension appears before a dimension whose normal Rill name is already the timeseries column, e.g. dimensions: [{expression: order_date}, {column: order_date}], this rename gives both dimensions the name order_date. Rill's fallback would keep the first dimension as dimension_0, but the imported model now fails validate_model/add_model with a duplicate dimension name, so the importer rejects an otherwise parseable view; consider checking all existing/future derived names before claiming the timeseries name.
Useful? React with 👍 / 👎.
1 participant
Follow-up to #211.
A Rill metrics view can declare
timeseries: order_datealongside an unnamed expression dimension whose expression is that same column (e.g.dimensions: [{expression: order_date}]). The name fallback in_parse_dimensionderiveddimension_0for it, so:dimension_0;sql == timeseries_columnand skipped creating anorder_datedimension;default_time_dimensionstayedorder_date, which referenced no dimension, sovalidate_modelrejected the model.Fix: when an unnamed dimension's SQL expression is the timeseries column, name it after the timeseries column so the generated time dimension stays addressable and
default_time_dimensionresolves.Scope: only affects the unnamed-dimension name fallback. Explicitly named dimensions and the
column/property/lookup_key_columnderivations are unchanged; the auto-create-when-missing path still applies when no dimension references the timeseries column.Adds two regression tests in
tests/adapters/rill/test_modern_features.pycovering parse-level addressability/validation and the CLI-firstadd_modelimport path.