Add meta to measures, entities, and dimensions #358

DevonFulcher · 2024-10-23T17:01:32Z

Description

This PR is modeled off of this prior PR. This PR adds meta to measures, entities, and dimensions.

Checklist

I have read the contributing guide and understand what's expected of me
I have signed the CLA
This PR includes tests, or tests are not required/relevant for this PR
I have run changie new to create a changelog entry

courtneyholcomb

Looks good! Left one main point of feedback.

Would love to also get @WilliamDee 's eyes on this just in case because I'm not super familiar with how meta & config are supposed to look.

Also - whenever we make a schema change, we get the core team's approval first. I'm not sure if that's necessary here since it's just copying what exists on a metric already, but I would recommend asking Jordan to confirm if we should get their eyes on this before merging.

courtneyholcomb · 2024-10-23T19:02:34Z

dbt_semantic_interfaces/implementations/elements/dimension.py

@@ -37,6 +42,16 @@ class PydanticDimensionTypeParams(HashableBaseModel):
    validity_params: Optional[PydanticDimensionValidityParams] = None


+class PydanticDimensionConfig(HashableBaseModel, ProtocolHint[DimensionConfig]):


If these protocols & implementations are all exactly the same structure, I think we should just use the MetricConfig protocol & implementation for all of them. Would be nice to remove the duplicate code if it's not needed. @WilliamDee would you foresee any problems with that?
It would be great if we could rename the class to just something more generic like SemanticLayerElementConfig or something if we're going to use it on all these elements. That would technically be a breaking change for DSI, but I just checked in dbt-core and it's not used anywhere. So it should be fine.
We would want to bump DSI to the next minor version if we do that. So if we do that, I can walk you through the steps for that.

We could do that, but a foreseeable issue comes when or if the Config ever diverges for different elements. Then in that case it may be abit of a pain in the ass to change.

actually, seems like core just uses config as some arbitrary dict, https://github.com/dbt-labs/dbt-core/blob/main/core/dbt/contracts/graph/unparsed.py#L615 so i think it's fine to generalize it in DSI

Okay, I made this update for the resources in this PR. I will make a follow-up PR to consolidate the other resources. I will wait on Grace's approval to merge this PR.

WilliamDee

this all looks good to me! Just need to resolve the Courtney's comment of whether we want to consolidate the Config to a single class

courtneyholcomb

Requested one more change!

dbt_semantic_interfaces/protocols/meta.py

courtneyholcomb · 2024-10-24T18:50:06Z

Also, do you mind searching dbt-mantle for the MetricConfig classes that will be removed to make sure they aren't used there? Both the protocol and the implementation. I already checked core.

courtneyholcomb · 2024-10-24T18:51:54Z

Also, do you mind searching dbt-mantle for the MetricConfig classes that will be removed to make sure they aren't used there? Both the protocol and the implementation. I already checked core.

@DevonFulcher I also didn't realize we had config on semantic models too! So can you check both dbt-core and dbt-mantle to make sure all the SemanticModelConfig and MetricConfig classes aren't used?

DevonFulcher · 2024-10-24T19:15:45Z

I also didn't realize we had config on semantic models too! So can you check both dbt-core and dbt-mantle to make sure all the SemanticModelConfig and MetricConfig classes aren't used?

@courtneyholcomb Based on my search, it looks like mantle and core have classes with the same name as SemanticModelConfig and MetricConfig, but those repos are not importing these classes from dsi. Also, to be clear, this PR shouldn't contain any breaking changes. That is my other PR here.

courtneyholcomb · 2024-10-24T19:28:00Z

I also didn't realize we had config on semantic models too! So can you check both dbt-core and dbt-mantle to make sure all the SemanticModelConfig and MetricConfig classes aren't used?

@courtneyholcomb Based on my search, it looks like mantle and core have classes with the same name as SemanticModelConfig and MetricConfig, but those repos are not importing these classes from dsi. Also, to be clear, this PR shouldn't contain any breaking changes. That is my other PR here.

@DevonFulcher Sounds like we're good then!! And yeah, sorry, I didn't put this in the appropriate PR 😆

dbt_semantic_interfaces/implementations/element_config.py

theyostalservice · 2024-11-05T23:12:58Z

dbt_semantic_interfaces/implementations/element_config.py

+    def _implements_protocol(self) -> SemanticLayerElementConfig:  # noqa: D
+        return self
+
+    meta: Dict[str, Any] = Field(default_factory=dict)


Would this work if we add more fields to the config and wanted to treat meta as optional? (For example, if we wanted to allow someone to add either some metadata or a tag.)

(This is not necessarily a blocking question.)

Good point! We can make this optional in the future, though, without a breaking change.

Is this not already optional? In a sense that, it'll default to an empty dict if you didn't provide it anything

Ya, it is effectively optional because of the default, although not type Optional.

### Description This PR addresses [this](#358 (comment)) feedback to consolidate the meta config into central models. This is technically a breaking change because some classes have been deleted, but Courtney confirmed that these classes are not being used by dbt-core. ### Checklist - [x] I have read [the contributing guide](https://github.com/dbt-labs/dbt-semantic-interfaces/blob/main/CONTRIBUTING.md) and understand what's expected of me - [x] I have signed the [CLA](https://docs.getdbt.com/docs/contributor-license-agreements) - [x] This PR includes tests, or tests are not required/relevant for this PR - [x] I have run `changie new` to [create a changelog entry](https://github.com/dbt-labs/dbt-semantic-interfaces/blob/main/CONTRIBUTING.md#adding-a-changelog-entry)

cla-bot bot added the cla:yes label Oct 23, 2024

Add meta to measures, entities, and dimensions

f6fea48

DevonFulcher force-pushed the Add_meta_to_measures,_entities,_and_dimensions branch from e155c0d to f6fea48 Compare October 23, 2024 17:30

DevonFulcher marked this pull request as ready for review October 23, 2024 17:34

DevonFulcher requested review from QMalcolm, graciegoheen and a team as code owners October 23, 2024 17:34

DevonFulcher requested a review from courtneyholcomb October 23, 2024 17:34

courtneyholcomb reviewed Oct 23, 2024

View reviewed changes

WilliamDee reviewed Oct 23, 2024

View reviewed changes

Consolidated meta to use SemanticLayerElementConfig

44daf59

DevonFulcher mentioned this pull request Oct 24, 2024

Consolidated meta config to use the same models #361

Merged

4 tasks

courtneyholcomb reviewed Oct 24, 2024

View reviewed changes

dbt_semantic_interfaces/protocols/meta.py Outdated Show resolved Hide resolved

Rename to PydanticSemanticLayerElementConfig

ada4b16

courtneyholcomb reviewed Oct 24, 2024

View reviewed changes

dbt_semantic_interfaces/implementations/element_config.py Outdated Show resolved Hide resolved

Fix naming

79916e8

DevonFulcher mentioned this pull request Nov 5, 2024

[Feature] Support for meta in Dimensions, Entities, and Measures #362

Open

3 tasks

theyostalservice reviewed Nov 5, 2024

View reviewed changes

WilliamDee approved these changes Nov 11, 2024

View reviewed changes

Merge branch 'main' into Add_meta_to_measures,_entities,_and_dimensions

45a0b52

DevonFulcher merged commit c833632 into main Nov 12, 2024
22 checks passed

DevonFulcher deleted the Add_meta_to_measures,_entities,_and_dimensions branch November 12, 2024 19:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add meta to measures, entities, and dimensions #358

Add meta to measures, entities, and dimensions #358

DevonFulcher commented Oct 23, 2024 •

edited

Loading

courtneyholcomb left a comment

courtneyholcomb Oct 23, 2024

WilliamDee Oct 23, 2024

WilliamDee Oct 23, 2024

DevonFulcher Oct 24, 2024

WilliamDee left a comment

courtneyholcomb left a comment

courtneyholcomb commented Oct 24, 2024

courtneyholcomb commented Oct 24, 2024

DevonFulcher commented Oct 24, 2024

courtneyholcomb commented Oct 24, 2024

theyostalservice Nov 5, 2024

DevonFulcher Nov 6, 2024

WilliamDee Nov 6, 2024

DevonFulcher Nov 6, 2024 •

edited

Loading

		@@ -37,6 +42,16 @@ class PydanticDimensionTypeParams(HashableBaseModel):
		validity_params: Optional[PydanticDimensionValidityParams] = None


		class PydanticDimensionConfig(HashableBaseModel, ProtocolHint[DimensionConfig]):

Add meta to measures, entities, and dimensions #358

Add meta to measures, entities, and dimensions #358

Conversation

DevonFulcher commented Oct 23, 2024 • edited Loading

Description

Checklist

courtneyholcomb left a comment

Choose a reason for hiding this comment

courtneyholcomb Oct 23, 2024

Choose a reason for hiding this comment

WilliamDee Oct 23, 2024

Choose a reason for hiding this comment

WilliamDee Oct 23, 2024

Choose a reason for hiding this comment

DevonFulcher Oct 24, 2024

Choose a reason for hiding this comment

WilliamDee left a comment

Choose a reason for hiding this comment

courtneyholcomb left a comment

Choose a reason for hiding this comment

courtneyholcomb commented Oct 24, 2024

courtneyholcomb commented Oct 24, 2024

DevonFulcher commented Oct 24, 2024

courtneyholcomb commented Oct 24, 2024

theyostalservice Nov 5, 2024

Choose a reason for hiding this comment

DevonFulcher Nov 6, 2024

Choose a reason for hiding this comment

WilliamDee Nov 6, 2024

Choose a reason for hiding this comment

DevonFulcher Nov 6, 2024 • edited Loading

Choose a reason for hiding this comment

DevonFulcher commented Oct 23, 2024 •

edited

Loading

DevonFulcher Nov 6, 2024 •

edited

Loading