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

Metric filters for distinct values queries #1107

Merged
merged 9 commits into from
Apr 9, 2024
Merged

Metric filters for distinct values queries #1107

merged 9 commits into from
Apr 9, 2024

Conversation

courtneyholcomb
Copy link
Contributor

@courtneyholcomb courtneyholcomb commented Mar 29, 2024 •
edited
Loading

Completes SL-1939

Description

Enable metric filters for distinct values queries (queries without metrics).

@cla-bot cla-bot bot added the cla:yes label Mar 29, 2024
@courtneyholcomb courtneyholcomb force-pushed the court/metric-filters-for-distinct-values-queries branch 3 times, most recently from 6dda541 to a34b3b3 Compare March 30, 2024 01:26
@linear Linear
Copy link

linear bot commented Mar 30, 2024

SL-1939 Allow metrics in filters for distinct values queries

@dbt-labs dbt-labs deleted a comment from github-actions bot Mar 30, 2024
@courtneyholcomb courtneyholcomb marked this pull request as ready for review March 30, 2024 01:31
@courtneyholcomb courtneyholcomb added the Run Tests With Other SQL Engines Runs the test suite against the SQL engines in our target environment label Mar 30, 2024
@courtneyholcomb courtneyholcomb force-pushed the court/metric-filters-for-distinct-values-queries branch from d028188 to fe30ab3 Compare March 30, 2024 01:45
@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 Mar 30, 2024
@courtneyholcomb courtneyholcomb temporarily deployed to DW_INTEGRATION_TESTS March 30, 2024 01:49 — with GitHub Actions Inactive
@courtneyholcomb courtneyholcomb temporarily deployed to DW_INTEGRATION_TESTS March 30, 2024 01:49 — with GitHub Actions Inactive
@courtneyholcomb courtneyholcomb temporarily deployed to DW_INTEGRATION_TESTS March 30, 2024 01:49 — with GitHub Actions Inactive
@courtneyholcomb courtneyholcomb temporarily deployed to DW_INTEGRATION_TESTS March 30, 2024 01:49 — 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 Mar 30, 2024
@tlento
Copy link
Contributor

tlento commented Apr 2, 2024

Trino is once again unhappy about something. Another non-ANSI but broadly-supported group by expression, perhaps?

@courtneyholcomb courtneyholcomb force-pushed the court/group-by-metric-dfp branch from b2b0efe to c3c16ab Compare April 2, 2024 02:06
@courtneyholcomb courtneyholcomb force-pushed the court/metric-filters-for-distinct-values-queries branch from fe30ab3 to bd8a335 Compare April 2, 2024 02:08
@courtneyholcomb courtneyholcomb added the Run Tests With Other SQL Engines Runs the test suite against the SQL engines in our target environment label Apr 2, 2024
@courtneyholcomb courtneyholcomb temporarily deployed to DW_INTEGRATION_TESTS April 2, 2024 02:10 — with GitHub Actions Inactive
@courtneyholcomb courtneyholcomb temporarily deployed to DW_INTEGRATION_TESTS April 2, 2024 02:10 — with GitHub Actions Inactive
@courtneyholcomb courtneyholcomb temporarily deployed to DW_INTEGRATION_TESTS April 2, 2024 02:10 — with GitHub Actions Inactive
@courtneyholcomb courtneyholcomb temporarily deployed to DW_INTEGRATION_TESTS April 2, 2024 02:10 — with GitHub Actions Inactive
@courtneyholcomb
Copy link
Contributor Author

Trino is once again unhappy about something. Another non-ANSI but broadly-supported group by expression, perhaps?

@tlento DANGIT. Updated!

@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 Apr 2, 2024
Base automatically changed from court/group-by-metric-dfp to main April 2, 2024 19:48
@tlento
Copy link
Contributor

tlento commented Apr 4, 2024

Argh, this fell off my radar, sorry.

Is this commit set correct? It looks like you need a rebase, I'm seeing things that look like duplicates of earlier work. If so, you can just point me to the first thing I should read and I can go from there while the PR itself gets fixed up.

@courtneyholcomb
Copy link
Contributor Author

courtneyholcomb commented Apr 4, 2024 •
edited
Loading

Argh, this fell off my radar, sorry.

Is this commit set correct? It looks like you need a rebase, I'm seeing things that look like duplicates of earlier work. If so, you can just point me to the first thing I should read and I can go from there while the PR itself gets fixed up.

Dangit you're right! The first commit to look at is Add METRIC property to all_properties. Thank you!!
@tlento

@courtneyholcomb courtneyholcomb force-pushed the court/metric-filters-for-distinct-values-queries branch 2 times, most recently from d495014 to 4f0fb54 Compare April 4, 2024 19:15
@courtneyholcomb courtneyholcomb added the Run Tests With Other SQL Engines Runs the test suite against the SQL engines in our target environment label Apr 4, 2024
@courtneyholcomb courtneyholcomb temporarily deployed to DW_INTEGRATION_TESTS April 4, 2024 19:15 — with GitHub Actions Inactive
@courtneyholcomb courtneyholcomb temporarily deployed to DW_INTEGRATION_TESTS April 4, 2024 19:15 — with GitHub Actions Inactive
@courtneyholcomb courtneyholcomb temporarily deployed to DW_INTEGRATION_TESTS April 4, 2024 19:15 — with GitHub Actions Inactive
@courtneyholcomb courtneyholcomb temporarily deployed to DW_INTEGRATION_TESTS April 4, 2024 19:15 — 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 Apr 4, 2024
@courtneyholcomb courtneyholcomb mentioned this pull request Apr 8, 2024
Merged
@courtneyholcomb courtneyholcomb force-pushed the court/metric-filters-for-distinct-values-queries branch from 4f0fb54 to 2d45143 Compare April 9, 2024 17:34
tlento
tlento approved these changes Apr 9, 2024
View reviewed changes
Copy link
Contributor

@tlento tlento left a comment

Choose a reason for hiding this comment

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

Forgotten no more! Sorry about that, every day I was like "oh I'll do this before I stop today" and every day I filled my brain with dataflow plan builder implementation details and forgot all about everything else, so I'm going back to the review first, code later order of operations.

tests/integration/test_cases/itest_dimensions.yaml Outdated
---
integration_test:
name: distinct_values_query_with_metric_filter
description: Query withought metrics using a metric filter
Copy link
Contributor

Choose a reason for hiding this comment

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

micro-nit: without

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Idk I kinda like my creative spelling.

metricflow/dataflow/builder/dataflow_plan_builder.py
filter_location=WhereFilterLocation.for_query(
tuple(metric_spec.reference for metric_spec in query_spec.metric_specs)
),
filter_location=WhereFilterLocation.for_query(metric_references=tuple()),
Copy link
Contributor

Choose a reason for hiding this comment

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

You know, it took me way too long to figure out why there were no metric specs in this query. 🤦

@courtneyholcomb courtneyholcomb merged commit 94ead4c into main Apr 9, 2024
13 checks passed
@courtneyholcomb courtneyholcomb deleted the court/metric-filters-for-distinct-values-queries branch April 9, 2024 21:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tlento tlento tlento approved these changes

@plypaul plypaul Awaiting requested review from plypaul

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 @tlento