-
Notifications
You must be signed in to change notification settings - Fork 98
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
Add Check to Verify queried_semantic_models
#1207
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great test to add 🙏 Thank you!
@@ -361,3 +362,16 @@ def test_case( | |||
) | |||
# If we sort, it's effectively not checking the order whatever order that the output was would be overwritten. | |||
assert_dataframes_equal(actual, expected, sort_columns=not case.check_order, allow_empty=case.allow_empty) | |||
|
|||
# Check that the parse result and the dataflow plan show the same semantic models read. | |||
if name in {"itest_dimensions.yaml/distinct_values_query_with_metric_filter"}: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@plypaul did some digging and it turns out this bug is not in the metric filters feature, it's in queries without metrics in general. You can see this test case gets the same error. I'll put up an issue to fix that, but you might want to update the message below to reflect that.
a740810
to
a784bdf
Compare
4feb4d6
to
04a0a16
Compare
Description
This PR adds a check to the integration test cases to verify that the queried semantic models returned by the parser match the semantic models queried in the dataflow plan.
Tagging @courtneyholcomb as there's currently a case that is skipped - do you have context offhand why that's the case?