Skip to content

Conversation

@theyostalservice
Copy link
Contributor

@theyostalservice theyostalservice commented Oct 1, 2025

Towards #387

Description

This PR contains a few small changes and improvements, all grouped up in the process of getting to a point where metricflow can onboard onto the new dsi version immediately.

At a high level, these changes take three forms. I've tried to detail in comments throughout the PR, but here as well:

  • general clean up or improvements that came up along the way
  • fixing the equality checking when searching for matching metrics (to avoid creating duplicates).
    • this is a correctness issue here in this repo. It can arise when we have a metric created from a measure with create_metric=true and then also create one for inputs. (This was found in some of the "parse-the-entire-project" tests in metricflow.)
  • adding measures and sometimes input_measures to some of the new metrics made in transformations. This is only necessary because the upgrades in MF are not yet complete and some operations still throw errors when encountering metrics without input measures (especially in tests :/ ), and these new lines can be removed in a few weeks when everything is ready.

I've also tested this by pointing the metricflow repo at the top commit here to make sure make test is running alright since the latest dsi version would not work in MF before.

Checklist

@cla-bot cla-bot bot added the cla:yes label Oct 1, 2025
@github-actions
Copy link

github-actions bot commented Oct 1, 2025

Thank you for your pull request! We could not find a changelog entry for this change. For details on how to document a change, see the contributing guide.

Copy link
Contributor Author

@github-actions
Copy link

github-actions bot commented Oct 1, 2025

Thank you for your pull request! We could not find a changelog entry for this change. For details on how to document a change, see the contributing guide.

@theyostalservice theyostalservice force-pushed the patricky/add_measure_to_new_metrics_01 branch from 6fc1365 to b5f1efc Compare October 2, 2025 14:27
@theyostalservice theyostalservice force-pushed the patricky/add_measure_to_new_metrics_01 branch from b5f1efc to e35ed6e Compare October 2, 2025 15:17
type_params:
measure:
name: bookings
fill_nulls_with: 15
Copy link
Contributor Author

Choose a reason for hiding this comment

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

de-duplicating cut the number of metrics down by 1, but I wanted to make sure we had variety in our tests so I made this one require a new metric.

Copy link
Contributor

Choose a reason for hiding this comment

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

what a silly metric!

@theyostalservice theyostalservice changed the title Add measures to metrics (temporary) Add measures to metrics (temporary) & fix metric deduplication Oct 2, 2025
@theyostalservice theyostalservice marked this pull request as ready for review October 2, 2025 19:42
@theyostalservice theyostalservice requested a review from a team as a code owner October 2, 2025 19:42
Copy link
Contributor

@courtneyholcomb courtneyholcomb left a comment

Choose a reason for hiding this comment

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

One main concern!

type_params:
measure:
name: bookings
fill_nulls_with: 15
Copy link
Contributor

Choose a reason for hiding this comment

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

what a silly metric!

Copy link
Contributor

@courtneyholcomb courtneyholcomb left a comment

Choose a reason for hiding this comment

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

lgtm!

Copy link
Contributor Author

theyostalservice commented Oct 3, 2025

Merge activity

  • Oct 3, 12:15 AM UTC: A user started a stack merge that includes this pull request via Graphite.
  • Oct 3, 12:15 AM UTC: @theyostalservice merged this pull request with Graphite.

@theyostalservice theyostalservice merged commit 3ff5ec3 into main Oct 3, 2025
20 checks passed
@theyostalservice theyostalservice deleted the patricky/add_measure_to_new_metrics_01 branch October 3, 2025 00:15
theyostalservice added a commit to dbt-labs/metricflow that referenced this pull request Oct 3, 2025
Version BUUUUUUUMP

This PR is just here to pull in the new transformations and such from dsi.  Early commits in this PR will be pinned to commits while I'm working on feeding dbt-labs/dbt-semantic-interfaces#436 to the merge-asaurus.

Snapshot updates reflect the fact that now(*) we produce more metrics to replace measures, reflecting the changes in structure for the new user YAML.

(*) "now" is true, but ONLY IN TESTS.  Because of a transformation rule that exists only in MF right now, we don't use the list of transformations directly from dsi.  See upstack #1877 for the PR that brings these changes to prod.  OTH, tests DO run on the default list of metric transformations from dsi, so the snapshots needed more immediate updating.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants