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

[SPIKE] [CT-2669] Revisit NodeRelation #7823

Closed
Tracked by #7498
peterallenwebb opened this issue Jun 7, 2023 · 4 comments
Closed
Tracked by #7498

[SPIKE] [CT-2669] Revisit NodeRelation #7823

peterallenwebb opened this issue Jun 7, 2023 · 4 comments
Assignees

Comments

@peterallenwebb
Copy link
Contributor

peterallenwebb commented Jun 7, 2023

Several concerns have been raised about the NodeRelation class, which is currently part of the semantic interfaces protocol for SemanticModel. See this comment for the full discussion, but the core issues are:

  • Should the NodeRelation just be replaced with a single string representing the fully qualified and appropriately quoted relation name? Otherwise should it be combined somehow with the existing StateRelation class?
  • How will we coordinate these changes with changes to the dbt-semantic-interfaces? This is an interesting early test case for changes that need coordination between it and dbt-core.
@github-actions github-actions bot changed the title [Feature] <title> [CT-2669] [Feature] <title> Jun 7, 2023
@peterallenwebb peterallenwebb changed the title [CT-2669] [Feature] <title> [CT-2669] Revisit NodeRelation Jun 7, 2023
@jtcohen6
Copy link
Contributor

Should the NodeRelation just be replaced with a single string representing the fully qualified and appropriately quoted relation name?

Not either/or, but yes/and! We should include both the three components (database, schema, identifier) and the stringified/quoted relation_name. Similar to what we do for logging (although there we do call it alias instead of identifier).

// NodeRelation
message NodeRelation {
string database = 10;
string schema = 11;
string alias = 12;
string relation_name = 13;
}

Otherwise should it be combined somehow with the existing StateRelation class?

I agree there's some significant overlap between these, and they would ideally be consistent!

@emmyoop emmyoop added the spike label Jun 12, 2023
@emmyoop emmyoop changed the title [CT-2669] Revisit NodeRelation [SPIKE] [CT-2669] Revisit NodeRelation Jun 12, 2023
@jtcohen6
Copy link
Contributor

jtcohen6 commented Jul 5, 2023

@peterallenwebb What's the latest state of this conversation with the MetricFlow / DSI team? My sense is that's what's blocking us from making this change, not any actual complexity of the change within dbt-core

@peterallenwebb
Copy link
Contributor Author

@jtcohen6 Sorry for the late update, but we went over final release strategy with the MetricFlow team and we plan to get this change into Core, along with a coordinated change in dbt-semantic-interfaces late today or early tomorrow.

@peterallenwebb
Copy link
Contributor Author

We decided against any changes here. It would have been nice to rename the schema_name property on NodeRelation to schema, for greater consistency in the manifest JSON, but Pydantic reserves the field name schema which would have complicated the changes in dsi-semantic-interfaces considerably. The benefits outweigh the costs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants