Skip to content

Conversation

theyostalservice
Copy link
Contributor

@theyostalservice theyostalservice commented Sep 29, 2025

Towards #387

Description

Depends on #432

We already convert measures with agg value MEDIAN to be percentile measures. Since we're replacing measures with simple metrics, this PR does the same for simple metrics.

Notes:

  • Learning from several previous PRs, including especially Add convert-count rule for metrics, too #431 , we use some inheritance to keep all the good, non-legacy code in the transformation class for metrics while still allowing a lot of reuse for the measures-related class.
  • I also did a little renaming and re-documenting to the test cases I added to show make it clear what was legacy and for measures and what was not.

Checklist

@cla-bot cla-bot bot added the cla:yes label Sep 29, 2025
Copy link

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

theyostalservice commented Sep 29, 2025

@theyostalservice theyostalservice force-pushed the patricky/convert-median-for-metrics-too branch 2 times, most recently from 1da4cc1 to 6c223f5 Compare September 29, 2025 21:37
@theyostalservice theyostalservice marked this pull request as ready for review September 29, 2025 21:39
@theyostalservice theyostalservice requested a review from a team as a code owner September 29, 2025 21:39
Copy link
Contributor Author

theyostalservice commented Sep 30, 2025

Merge activity

  • Sep 30, 3:02 PM UTC: A user started a stack merge that includes this pull request via Graphite.
  • Sep 30, 3:04 PM UTC: Graphite rebased this pull request as part of a merge.
  • Sep 30, 3:05 PM UTC: @theyostalservice merged this pull request with Graphite.

@theyostalservice theyostalservice changed the base branch from patricky/median-measure-transformation-tests to graphite-base/433 September 30, 2025 15:02
theyostalservice added a commit that referenced this pull request Sep 30, 2025
Towards #387

### Description

We already convert measures with agg value MEDIAN to be percentile measures.  Since we're replacing measures with simple metrics, #433 will do the same thing for simple metrics.  Before that, it'd be helpful to have a test in place that shows that the existing legacy measure conversion work, and that's what this PR is for!

### Checklist

- [x] I have read [the contributing guide](https://github.com/dbt-labs/dbt-semantic-interfaces/blob/main/CONTRIBUTING.md) and understand what's expected of me
- [x] I have signed the [CLA](https://docs.getdbt.com/docs/contributor-license-agreements)
- [x] This PR includes tests, or tests are not required/relevant for this PR
- [ ] I have run `changie new` to [create a changelog entry](https://github.com/dbt-labs/dbt-semantic-interfaces/blob/main/CONTRIBUTING.md#adding-a-changelog-entry)
@theyostalservice theyostalservice changed the base branch from graphite-base/433 to main September 30, 2025 15:02
@theyostalservice theyostalservice force-pushed the patricky/convert-median-for-metrics-too branch from 6c223f5 to 15092b6 Compare September 30, 2025 15:03
@theyostalservice theyostalservice merged commit 336f22d into main Sep 30, 2025
20 checks passed
@theyostalservice theyostalservice deleted the patricky/convert-median-for-metrics-too branch September 30, 2025 15:05
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.

2 participants