Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Support conversion metrics with custom grain #1475

Merged
merged 9 commits into from
Oct 30, 2024
Merged

Support conversion metrics with custom grain #1475

merged 9 commits into from
Oct 30, 2024

Conversation

courtneyholcomb
Copy link
Contributor

@courtneyholcomb courtneyholcomb commented Oct 23, 2024 •
edited
Loading

Support for conversion metrics used with custom granularities in the group by or the where filter.

@cla-bot cla-bot bot added the cla:yes label Oct 23, 2024
@courtneyholcomb courtneyholcomb changed the base branch from main to will/fix-extra-linkable-specs October 23, 2024 23:16
@dbt-labs dbt-labs deleted a comment from github-actions bot Oct 23, 2024
Base automatically changed from will/fix-extra-linkable-specs to main October 25, 2024 02:40
@courtneyholcomb courtneyholcomb force-pushed the court/custom-conversion branch 2 times, most recently from a470bdb to fc171b4 Compare October 29, 2024 01:23
courtneyholcomb
courtneyholcomb commented Oct 29, 2024
View reviewed changes
metricflow-semantics/metricflow_semantics/specs/spec_set.py
from metricflow_semantics.specs.metric_spec import MetricSpec
from metricflow_semantics.specs.time_dimension_spec import TimeDimensionSpec

if TYPE_CHECKING:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Needed to resolve circular imports.

@courtneyholcomb courtneyholcomb force-pushed the court/custom-conversion branch 2 times, most recently from ab0e7fa to 9a41e79 Compare October 29, 2024 01:34
@courtneyholcomb
Update SourceNodeRecipe properties
2622e53 
Adds all_linkable_specs_required_for_source_nodes property. Storing this
 on SourceNodeRecipe allows us to keep logic for converting to base
 grain in one place instead of needing to propogate that in several
 places and add a comment explaining each one. This commit also adds a
 little bit of type cleanup to the same class.
@courtneyholcomb courtneyholcomb force-pushed the court/custom-conversion branch 2 times, most recently from 7576c0e to b636dd6 Compare October 29, 2024 01:40
courtneyholcomb
courtneyholcomb commented Oct 29, 2024
View reviewed changes
...y.py/SqlQueryPlan/DuckDB/test_conversion_metric_with_custom_granularity__plan0_optimized.sql
ORDER BY subq_28.ds__day DESC
ROWS BETWEEN UNBOUNDED PRECEDING AND UNBOUNDED FOLLOWING
) AS ds__day
, FIRST_VALUE(subq_28.metric_time__day) OVER (
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it expected that we still use metric_time__day in the interim calculations here? Looks like we use both metric_time__day and metric_time__martian_day in the window functions, even though only metric_time__martian_day was in the group by.

Copy link
Contributor

Choose a reason for hiding this comment

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

how does cumulative windows work for custom grains? We have metric_time__day here because that's what's used for the conversion window calculation (ie., look at line 126 below for the join condition). So metric_time__day is here because it's used for the conversion window and metric_time__martian_day is there because we will need it when grouping by it.

Copy link
Contributor

Choose a reason for hiding this comment

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

actually, looking at this, we use ds__day to do the calculation. There's probably some wonky thing happening with the column's I passed into the conversion node, not blocking, but I'll take a look.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cumulative windows can't use custom grains yet - that's part of the implementation we were discussing at MF team meeting today!
I think non-default grains are relevant, though. If you look at this SQL snapshot you can see an example of the window function we implement for that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But it sounds like the PR you put up today was a fix for what you mentioned here!

@courtneyholcomb courtneyholcomb marked this pull request as ready for review October 29, 2024 02:01
@courtneyholcomb courtneyholcomb added the Run Tests With Other SQL Engines Runs the test suite against the SQL engines in our target environment label Oct 29, 2024
@courtneyholcomb courtneyholcomb added Run Tests With Other SQL Engines Runs the test suite against the SQL engines in our target environment and removed Run Tests With Other SQL Engines Runs the test suite against the SQL engines in our target environment labels Oct 29, 2024
@github-actions github-actions bot removed the Run Tests With Other SQL Engines Runs the test suite against the SQL engines in our target environment label Oct 29, 2024
WilliamDee
WilliamDee approved these changes Oct 29, 2024
View reviewed changes
...y.py/SqlQueryPlan/DuckDB/test_conversion_metric_with_custom_granularity__plan0_optimized.sql
ORDER BY subq_28.ds__day DESC
ROWS BETWEEN UNBOUNDED PRECEDING AND UNBOUNDED FOLLOWING
) AS ds__day
, FIRST_VALUE(subq_28.metric_time__day) OVER (
Copy link
Contributor

Choose a reason for hiding this comment

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

how does cumulative windows work for custom grains? We have metric_time__day here because that's what's used for the conversion window calculation (ie., look at line 126 below for the join condition). So metric_time__day is here because it's used for the conversion window and metric_time__martian_day is there because we will need it when grouping by it.

.changes/unreleased/Features-20241023-161720.yaml Outdated
@@ -0,0 +1,6 @@
kind: Features
body: Support cumulative metrics queried with custom granularities.
Copy link
Contributor

Choose a reason for hiding this comment

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

Support conversion** metrics

WilliamDee
WilliamDee reviewed Oct 29, 2024
View reviewed changes
tests_metricflow/integration/test_cases/itest_granularity.yaml Outdated
{{ render_date_trunc("ds", TimeGranularity.DAY) }} AS ds__day
, user_id AS user
, 1 AS buys
, GEN_RANDOM_UUID() AS mf_internal_uuid
Copy link
Contributor

Choose a reason for hiding this comment

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

Need to swap GEN_RANDOM_UUID() with {{ generate_random_uuid() }}

@courtneyholcomb courtneyholcomb added the Run Tests With Other SQL Engines Runs the test suite against the SQL engines in our target environment label Oct 30, 2024
@github-actions github-actions bot removed the Run Tests With Other SQL Engines Runs the test suite against the SQL engines in our target environment label Oct 30, 2024
@courtneyholcomb courtneyholcomb merged commit c611b44 into main Oct 30, 2024
29 checks passed
@courtneyholcomb courtneyholcomb deleted the court/custom-conversion branch October 30, 2024 01:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@WilliamDee WilliamDee WilliamDee approved these changes

Assignees
No one assigned
Labels
cla:yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@courtneyholcomb @WilliamDee