Import metric reference entries in Holistics datasets#228
Conversation
The reusable-metric-store shorthand `metric name: standalone_metric` references a top-level Metric by name instead of redefining it inline. The parser represents that reference as a property rather than a block, so the dataset-block loop dropped it. A Dataset that extends a PartialDataset whose only contribution is such a reference then produced no model at all. Resolve metric-reference property entries against the standalone Metric registry (renamed to the dataset-local key) so the dataset still surfaces with the referenced metric attached.
![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: f53b3671a4
ℹ️ 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".
When a Dataset/PartialDataset authored inside a module references a bare standalone metric name via the reusable-metric-store shorthand, resolve the context-qualified name (e.g. finance.revenue) before the bare name. Previously the unqualified candidate was tried first, so a finance partial referencing revenue resolved to a same-named root metric instead of the local one, giving the extending dataset the wrong SQL.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 18b824932d
ℹ️ 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".
…in Holistics Register standalone Metric definitions at graph scope before dataset metrics. A dataset can carry a copy of a standalone metric renamed to a local alias via the metric name: standalone_metric shorthand; registering dataset metrics first let that alias occupy a same-named root metric's bare graph key and silently drop the genuine standalone metric.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6a195084f9
ℹ️ 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 reusable-reference shorthand (metric name: ref) is emitted by the
grammar as a bare metric keyword followed by a property, which _merge_blocks
keyed separately from a metric name { ... } block. An override authored in a
different form than the base was appended as a duplicate instead of replacing
it, and Model.get_metric returns the first match, so .extend() kept the base
SQL. Group named metric/measure/dimension blocks and reference shorthands under
a shared (keyword, name) field key so an override in either form wins.
nicosuave
force-pushed
the
fix-holistics-dataset-metric-references
branch
from
b6a08b0 to
20f9a1b
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 20f9a1bb60
ℹ️ 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".
_parse_dataset_block resolved every dataset AmlProperty whose value named a standalone metric as the metric name: ref shorthand, ignoring the bare metric keyword marker the grammar emits before a real reference. An ordinary metadata property such as label: some_identifier whose value happened to match a standalone metric name was misread as a reference, adding a bogus metric and surfacing a metadata-only dataset as a model. Only resolve references for properties actually preceded by the metric/measure marker.
1 participant
Follow-up to #204.
What & why
The Holistics reusable-metric-store shorthand
metric name: standalone_metricreferences a top-levelMetricby name instead of redefining it inline. The parser represents that reference as a property (not a block), so the dataset-block item loop insidemantic/adapters/holistics.pydropped it viacontinue.As a result, a
Dataset d = base.extend(partial_metrics)whose partial only contributes such metric references produced nodmodel at all, even though the referenced standaloneMetricexisted.PartialDataset.extendonly worked when every reusable metric was rewritten as a full inline block.This change resolves metric-reference property entries against the standalone
Metricregistry (copied and renamed to the dataset-local key) so the dataset still surfaces as a model carrying the referenced metric. Standalone metrics are now resolved before datasets so the reference can be resolved at dataset-parse time. Genuine dataset properties (label,description,models,relationships) are untouched because only values that resolve to a known standalone metric are pulled in.Known limitations
usealiases; a reference that cannot be matched to a known standalone metric is left as before (no spurious metric created).