Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Update core to support DSI 0.8.3 #10990
Update core to support DSI 0.8.3 #10990
Changes from all commits
57fd0fa
5879461
66fd87d
6fe69a0
7bf447a
074375b
7aae48d
363e44b
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Check warning on line 18 in core/dbt/artifacts/resources/v1/semantic_layer_components.py
Codecov / codecov/patch
core/dbt/artifacts/resources/v1/semantic_layer_components.py#L18
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.
Does DSI/Metricflow require these be lowercase now? If so, this may be breaking as for already parsed manifests 🤔
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.
Nope! It should not be case sensitive. This is lowercasing it before we get it so that the user doesn't need to worry about lowercasing it but we can expect lowercase on our side. Will added tests for case sensitivity in DSI, too.
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.
The words that concern me are
I'm not sure you can unfortunately, but I could be wrong about that. Could you point me to the tests around case sensitivity in DSI?
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.
Yep!
https://github.com/dbt-labs/dbt-semantic-interfaces/blob/f3a50f02b962ce7193b801784740e244057ddb69/tests/parsing/test_metric_parsing.py#L215-L216
and
https://github.com/dbt-labs/dbt-semantic-interfaces/blob/f3a50f02b962ce7193b801784740e244057ddb69/tests/parsing/test_metric_parsing.py#L240-L241
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.
Let me go back and make sure all other fields have this covered, too
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.
Alright @QMalcolm - sorry for the delay on this, had some other high prios and then didn't want to mess with code freeze! 🥶
We bumped the version of DSI one more time with some changes to fix the issues with case sensitivity. This is the PR with those changes, but these commits are most relevant: 1, 2.
Let us know if you think there are any other issues here! The thorough review was much appreciated 🙏
Once approved, we shouldn't merge quite yet - just because we want to coordinate the merge timing so that we will have time to update mantle between when this merges and the next sync to mantle.
cc @WilliamDee
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.
I think github screwed up those links somehow. They point to https://github.com/dbt-labs/dbt-core/pull/2083a249889252823b097098cb03471f2db0d00e and https://github.com/dbt-labs/dbt-core/pull/00abab69a8b4537abcc0ea9e544547e99a3c27a2. Which are commits, but on DSI, not core.
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.
For the public observer, the correct links are