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
Add SemanticModel Node Type #7769
Add SemanticModel Node Type #7769
Changes from all commits
17030ea
4b8d8a8
2fea3d6
9203b17
a79ad1f
27e13b8
84a47e1
85c4369
6bfc766
41ab90a
2406884
870402e
c410a65
a2de21a
6e70a2e
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Stu added in a
StateRelation
class, I think we can use that instead?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.
That one doesn't actually have the relation_name in it, and I think they want that. Settling on a common relation class is the "larger issue" that I mentioned we didn't need to solve at this particular moment.
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.
From this comment -- does this still need to include a unified relation_name that respects quoting + include policies?
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.
if we do -- we'll probably need the adapter.Relation to do something like this during compilation:
(from https://github.com/dbt-labs/dbt-core/blob/main/core/dbt/compilation.py#L486-L488)
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 don't think the SemanticModels actually get compiled, since they're yaml-only. There is a question of whether they need the individual pieces (identifier/schema/database) or just the relation_name...
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.
@QMalcolm I'm not sure of the answer to this, but I suspect the answer may be yes. What makes sense from the perspective of MetricFlow integration?
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.
@MichelleArk Yep! We need to include the unified relation_name, and it's desirable that it respects quoting + include policies.
As for whether we need both the individual pieces as well as the
relation_name
, for this PR yes, long term we don't have to. MetricFlow is fine with having the parts or the relation_name. It was suggested that we use this node_relation object as it was a pattern that was already used elsewhere, thus the shape of the object currently. Though MetricFlow currently uses therelation_name
attribute as seen here, and ignores the individual properties. However, the individual properties as well as therelation_name
are currently required by the protocol. So for the scope of this PR, we should include the individual properties as well as therelation_name
. Otherwise we'd first need to update and release a new DSI (which would include a number of other schema changes that have been made), and then propagate all those changes in core. Although thenode_relation
changes might be trivial in core, propagating the other changes with the new DSI could eat a fair bit of time.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.
this call can throw an
ExtractionError
(defined indbt_extractor
) -- would be good to catch it and give a throw user-facing error instead since it is likely to be a typoThere 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.
Agree, and we'll follow up in the current sprint with #7822.
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.
could we add a #TODO comment here that links to #7688? refs with the version keyword won't be supported until 7688 is closed. At that point we can pass in the parsed version instead of always passing
None
to the ref lookup callThere 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.
Agree, and we'll follow up in the current sprint with #7822.
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 my own understanding -- we need to resolve the relation later on because not all model nodes are available in the manifest for lookup at this point?
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.
Should we make the node relation evaluate lazily, so we can put its value in here?
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.
SemanticModels come from yaml only, and the referenced models should always be sql based and so already resolvable when this is parsed.
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.
@MichelleArk Yes, I discussed this with Gerda and she thought we could possibly handle it immediately, since models would likely have been created, but doing it at the end would be most likely to succeed without complications.
@aranke I'm can follow up the idea of lazy evaluation in the current sprint. If you have a specific method of doing it in mind can you add it as a comment to #7822?