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

Add Check to Verify queried_semantic_models #1207

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

Add Check to Verify queried_semantic_models #1207

wants to merge 2 commits into from

Conversation

plypaul
Copy link
Contributor

@plypaul plypaul commented May 11, 2024

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?

Copy link
Contributor

@courtneyholcomb courtneyholcomb left a 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"}:
Copy link
Contributor

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.

@plypaul plypaul changed the base branch from p--smr--05 to main June 17, 2024 19:45
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