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

Tests for Metric.time_granularity #1325

Merged
merged 8 commits into from
Jul 22, 2024
Merged

Tests for Metric.time_granularity #1325

merged 8 commits into from
Jul 22, 2024

Conversation

courtneyholcomb
Copy link
Contributor

@courtneyholcomb courtneyholcomb commented Jul 11, 2024 •
edited
Loading

Adds default_granularity tests for query parsing, SQL rendering, query output, and check query tests.
Also cleans up a couple of comments due to product decisions that changed the expected implementation of this feature.

I will add more tests once we finalize the expected behavior for more specific scenarios.

@cla-bot cla-bot bot added the cla:yes label Jul 11, 2024
@courtneyholcomb courtneyholcomb requested a review from plypaul July 11, 2024 16:55
@courtneyholcomb courtneyholcomb marked this pull request as ready for review July 11, 2024 16:55
@courtneyholcomb courtneyholcomb added the Run Tests With Other SQL Engines Runs the test suite against the SQL engines in our target environment label Jul 11, 2024
@courtneyholcomb courtneyholcomb temporarily deployed to DW_INTEGRATION_TESTS July 11, 2024 17:12 — with GitHub Actions Inactive
@courtneyholcomb courtneyholcomb temporarily deployed to DW_INTEGRATION_TESTS July 11, 2024 17:12 — with GitHub Actions Inactive
@courtneyholcomb courtneyholcomb temporarily deployed to DW_INTEGRATION_TESTS July 11, 2024 17:12 — with GitHub Actions Inactive
@courtneyholcomb courtneyholcomb temporarily deployed to DW_INTEGRATION_TESTS July 11, 2024 17:12 — with GitHub Actions Inactive
@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 Jul 11, 2024
@courtneyholcomb courtneyholcomb force-pushed the court/default-granularity3 branch from f9793a9 to 5091b26 Compare July 12, 2024 14:48
@courtneyholcomb courtneyholcomb force-pushed the court/default-granularity4 branch from ce71955 to 8666f5c Compare July 12, 2024 16:12
@courtneyholcomb courtneyholcomb changed the base branch from court/default-granularity3 to court/default-granularity-2 July 12, 2024 16:15
@courtneyholcomb courtneyholcomb force-pushed the court/default-granularity4 branch from aa64a98 to 454613d Compare July 13, 2024 03:24
@courtneyholcomb courtneyholcomb force-pushed the court/default-granularity-2 branch 2 times, most recently from 8e2d601 to 2a0bcb2 Compare July 15, 2024 02:54
@courtneyholcomb courtneyholcomb force-pushed the court/default-granularity4 branch from 454613d to 08f159b Compare July 15, 2024 02:55
@courtneyholcomb courtneyholcomb force-pushed the court/default-granularity-2 branch from 2a0bcb2 to 425080e Compare July 15, 2024 17:36
@courtneyholcomb courtneyholcomb force-pushed the court/default-granularity4 branch from 7e7120b to 2f0cdc0 Compare July 15, 2024 17:37
Base automatically changed from court/default-granularity-2 to main July 16, 2024 01:29
@courtneyholcomb courtneyholcomb force-pushed the court/default-granularity4 branch from 2f0cdc0 to 72e1986 Compare July 16, 2024 01:30
@plypaul
Copy link
Contributor

plypaul commented Jul 16, 2024

Following what you noted in the description - are you planning to add more items to this PR or has that already been done?

@courtneyholcomb
Copy link
Contributor Author

Following what you noted in the description - are you planning to add more items to this PR or has that already been done?

@plypaul planning to add them in a separate PR!

plypaul
plypaul reviewed Jul 16, 2024
View reviewed changes
Copy link
Contributor

@plypaul plypaul left a comment

Choose a reason for hiding this comment

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

Gotcha. In that case:

  • Is there a specific reason why the default granularity was updated for those metrics? It has resulted in a number of snapshot changes, so I am wondering if there's an interaction between those metrics and default granularity that's important to check.
  • For testing resolution, tests of the form in (

    metricflow/metricflow-semantics/tests_metricflow_semantics/query/test_ambiguous_entity_path.py

    Line 23 in a55c402

    def test_resolvable_ambiguous_entity_path( # noqa: D103
    ) might be more helpful instead of the style of tests in test_query_parser.py.
  • Does the DSI version bump need to go in this PR or can it be done in a separate PR?

@courtneyholcomb
Copy link
Contributor Author

@plypaul

  • Is there a specific reason why the default granularity was updated for those metrics? It has resulted in a number of snapshot changes, so I am wondering if there's an interaction between those metrics and default granularity that's important to check.

I chose to add time_granularity to those metrics to that we could see the behavior for a few different types of metrics that I thought might behave differently, like a simple metric vs. a metric with input metrics (the ratio metric).
The snapshot changes were intentional. I thought it would be helpful to include this feature in some tests that were not just unit tests. I went through the snapshot changes before putting up the PR before putting up the PR and thought they made sense based on the query inputs. What snapshot changes are you seeing that concern you?

Sure, I can add some tests there!

@courtneyholcomb courtneyholcomb changed the title Tests for default_granularity Tests for Metric.time_granularity Jul 16, 2024
@courtneyholcomb courtneyholcomb force-pushed the court/default-granularity4 branch from b794754 to cd211c6 Compare July 17, 2024 02:10
@courtneyholcomb courtneyholcomb force-pushed the court/default-granularity4 branch from cd211c6 to ec2a052 Compare July 17, 2024 02:11
@courtneyholcomb
Copy link
Contributor Author

@plypaul moved those query parser tests, and separated out the version bump into a new PR.

@courtneyholcomb courtneyholcomb requested a review from plypaul July 17, 2024 02:12
@plypaul
Copy link
Contributor

plypaul commented Jul 18, 2024

What snapshot changes are you seeing that concern you?

It was more that I was wondering whether there as any particular interaction between the default granularity and the fill configuration option.

plypaul
plypaul reviewed Jul 19, 2024
View reviewed changes
...ow_semantics/test_helpers/semantic_manifest_yamls/ambiguous_resolution_manifest/metrics.yaml Outdated
@@ -92,3 +92,31 @@ metric:
metrics:
- name: monthly_metric_0
filter: "{{ TimeDimension('metric_time') }} = '2020-01-01'"
---
metric:
name: simple_metric_with_time_granularity
Copy link
Contributor

Choose a reason for hiding this comment

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

More like ...with_default_time_granularity?

plypaul
plypaul approved these changes Jul 19, 2024
View reviewed changes
metricflow-semantics/tests_metricflow_semantics/query/test_query_parser.py Outdated
@@ -51,6 +51,10 @@
expr: "1"
agg: sum
create_metric: true
- name: monthly_bookings
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be removed now?

@courtneyholcomb
Copy link
Contributor Author

What snapshot changes are you seeing that concern you?

It was more that I was wondering whether there as any particular interaction between the default granularity and the fill configuration option.

Nope, there shouldn't be. Just happened to be that the metrics I added a time granularity to were used in those tests.

@courtneyholcomb courtneyholcomb merged commit e2c2271 into main Jul 22, 2024
15 checks passed
@courtneyholcomb courtneyholcomb deleted the court/default-granularity4 branch July 22, 2024 18:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@plypaul plypaul plypaul approved these changes

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

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