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.

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

Query suggestions for GroupByMetrics #1197

Merged
merged 6 commits into from
May 13, 2024
Merged

Conversation

courtneyholcomb
Copy link
Contributor

@courtneyholcomb courtneyholcomb commented May 10, 2024

Description

Enables suggestions for GroupByMetrics. This just means that, if you query an invalid group by metric, the suggestion generator will show you valid group by metrics to help you figure out the right filter.
More detail is provided in commit messages / commit descriptions.

@cla-bot cla-bot bot added the cla:yes label May 10, 2024
This pattern was filtering out group by metrics unintentionally.
@courtneyholcomb courtneyholcomb force-pushed the court/gbm-validations branch from 2952cf5 to a9a07e5 Compare May 13, 2024 15:51
@dbt-labs dbt-labs deleted a comment from github-actions bot May 13, 2024
@courtneyholcomb courtneyholcomb changed the title WIP - validations for GroupByMetrics Query suggestions for GroupByMetrics May 13, 2024
@courtneyholcomb courtneyholcomb changed the title Query suggestions for GroupByMetrics Query suggestions for GroupByMetrics May 13, 2024
@courtneyholcomb courtneyholcomb force-pushed the court/gbm-validations branch from a9a07e5 to db4bd40 Compare May 13, 2024 16:01
Currently, metric filters have a parameter that's not captured in their input string (the outer query entity links). This means that you might end up with multiple input strings that look the same, so we need to dedupe those. These duplicates will look different once we add a new parameter to metric filters that allows users to specify the outer entity links. But there should be no harm in using a set here, either way.
@courtneyholcomb courtneyholcomb force-pushed the court/gbm-validations branch 4 times, most recently from 9a74140 to 223b0c1 Compare May 13, 2024 17:55
GroupByMetrics are allowed in filters, but not in group by items. Use new pattern to distinguish the two.
…results

You'll notice some of the snapshots here have changed. The fuzzy match scores for these items have not changed. What changed was the order of candidate items passed into the similarity scorer once group by metrics were added to the list of candidate items. The change in order for these snapshots is due to ties in score that are returned in the order they came in. Sort inputs to ensure consistent results in the future.
@courtneyholcomb courtneyholcomb force-pushed the court/gbm-validations branch from 223b0c1 to fd3657e Compare May 13, 2024 17:59
@courtneyholcomb courtneyholcomb marked this pull request as ready for review May 13, 2024 17:59
@courtneyholcomb courtneyholcomb requested review from plypaul and tlento May 13, 2024 17:59
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.

Seems reasonable!

"""Tests that a query for an invalid group by metric gives an appropriate group by metric suggestion."""
with pytest.raises(InvalidQueryException, match="Metric\\('bookings', group_by=\\['listing'\\]\\)"):
bookings_query_parser.parse_and_validate_query(
metric_names=("bookings",), where_constraint_str="{{ Metric('listings', ['garbage']) }} > 1"
Copy link
Contributor

Choose a reason for hiding this comment

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

LOL. Where's the Oscar the Grouch garbage can emoji when I need one...

@@ -6,7 +6,7 @@ Error #1:
The given input does not exactly match any known metrics.

Suggestions:
['bookings', 'booking_fees', 'booking_value', 'instant_bookings', 'booking_payments', 'max_booking_value']
['bookings', 'booking_fees', 'booking_value', 'booking_payments', 'instant_bookings', 'booking_value_p99']
Copy link
Contributor

Choose a reason for hiding this comment

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

So max_booking_value was tied with booking_value_p99 here and the sort pushed the latter into 6th place?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well it's actually like a 7-way tie between a bunch of metrics.

@courtneyholcomb courtneyholcomb merged commit a446ffa into main May 13, 2024
15 checks passed
@courtneyholcomb courtneyholcomb deleted the court/gbm-validations branch May 13, 2024 20:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants