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.

Sign up for GitHub

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

Assorted Naming and Type Fixes #187

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Assorted Naming and Type Fixes #187

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions dbt_semantic_interfaces/implementations/metric.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,10 +28,10 @@ class PydanticMetricInputMeasure(PydanticCustomInputParser, HashableBaseModel):
"""

name: str
filter: Optional[PydanticWhereFilterIntersection]
alias: Optional[str]
join_to_timespine: bool = False
fill_nulls_with: Optional[int] = None
filter: Optional[PydanticWhereFilterIntersection] = None
alias: Optional[str] = None
time_spine_join: bool = False
null_fill_value: Optional[float] = None
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When we tried this float caused problems when the actual value is supposed to be an int.

In a perfect world we could do int | float but that doesn't appear to resolve correctly and we haven't figured out how to deal with it.


@classmethod
def _from_yaml_value(cls, input: PydanticParseableValueType) -> PydanticMetricInputMeasure:
Expand Down Expand Up @@ -124,7 +124,7 @@ class PydanticMetricInput(HashableBaseModel):
offset_to_grain: Optional[TimeGranularity]

@property
def as_reference(self) -> MetricReference:
def metric_reference(self) -> MetricReference:
"""Property accessor to get the MetricReference associated with this metric input."""
return MetricReference(element_name=self.name)

Expand Down
14 changes: 7 additions & 7 deletions dbt_semantic_interfaces/parsing/generated_json_schemas/default_explicit_schema.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,7 +211,7 @@
"expr": {
"type": [
"string",
"integer",
"number",
"boolean"
]
},
Expand Down Expand Up @@ -244,17 +244,17 @@
"alias": {
"type": "string"
},
"fill_nulls_with": {
"type": "integer"
},
"filter": {
"$ref": "#/definitions/filter_schema"
},
"join_to_timespine": {
"type": "boolean"
},
"name": {
"type": "string"
},
"null_fill_value": {
"type": "number"
},
"time_spine_join": {
"type": "boolean"
}
},
"type": "object"
Expand Down
6 changes: 3 additions & 3 deletions dbt_semantic_interfaces/parsing/schemas.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,8 +60,8 @@
"name": {"type": "string"},
"filter": {"$ref": "filter_schema"},
"alias": {"type": "string"},
"join_to_timespine": {"type": "boolean"},
"fill_nulls_with": {"type": "integer"},
"time_spine_join": {"type": "boolean"},
"null_fill_value": {"type": "number"},
},
"additionalProperties": False,
},
Expand Down Expand Up @@ -177,7 +177,7 @@
"type": "string",
"pattern": TRANSFORM_OBJECT_NAME_PATTERN,
},
"expr": {"type": ["string", "integer", "boolean"]},
"expr": {"type": ["string", "number", "boolean"]},
"agg_params": {"$ref": "aggregation_type_params_schema"},
"create_metric": {"type": "boolean"},
"create_metric_display_name": {"type": "string"},
Expand Down
8 changes: 4 additions & 4 deletions dbt_semantic_interfaces/protocols/metric.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,13 +46,13 @@ def post_aggregation_measure_reference(self) -> MeasureReference:

@property
@abstractmethod
def join_to_timespine(self) -> bool:
"""If the measure should be joined to the timespine."""
def time_spine_join(self) -> bool:
"""If the measure should be joined to the time spine."""
pass

@property
@abstractmethod
def fill_nulls_with(self) -> Optional[int]:
def null_fill_value(self) -> Optional[float]:
Comment on lines +49 to +55
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are hard breaking changes on the dbt-core parser so we can't push these into dbt-semantic-interfaces or take dependencies on these names in MetricFlow if we want to be able to backport things.

It's fine to add duplicate fields with the new names, and once we get support for both broadly available across the releases we are committed to supporting we can gradually remove the old ones, but given that 1) these names were chosen within the context of existing dbt metrics nomenclature and 2) they're now out in the world I'm not sure this adjustment is worth the effort.

"""What null values should be filled with if set."""
pass

Expand Down Expand Up @@ -102,7 +102,7 @@ def offset_to_grain(self) -> Optional[TimeGranularity]: # noqa: D

@property
@abstractmethod
def as_reference(self) -> MetricReference:
def metric_reference(self) -> MetricReference:
"""Property accessor to get the MetricReference associated with this metric input."""
...

Expand Down
18 changes: 9 additions & 9 deletions dbt_semantic_interfaces/references.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,14 +12,14 @@ class ElementReference(SerializableDataclass):
element_name: str


@dataclass(frozen=True)
@dataclass(frozen=True, order=True)
class LinkableElementReference(ElementReference):
"""Used when we need to refer to a dimension or entity, but other attributes are unknown."""

pass


@dataclass(frozen=True)
@dataclass(frozen=True, order=True)
class MeasureReference(ElementReference):
"""Used when we need to refer to a measure.

Expand All @@ -29,7 +29,7 @@ class MeasureReference(ElementReference):
pass


@dataclass(frozen=True)
@dataclass(frozen=True, order=True)
class DimensionReference(LinkableElementReference): # noqa: D
pass

Expand All @@ -38,20 +38,20 @@ def time_dimension_reference(self) -> TimeDimensionReference: # noqa: D
return TimeDimensionReference(element_name=self.element_name)


@dataclass(frozen=True)
@dataclass(frozen=True, order=True)
class EntityReference(LinkableElementReference): # noqa: D
pass


@dataclass(frozen=True)
@dataclass(frozen=True, order=True)
class TimeDimensionReference(DimensionReference): # noqa: D
pass

def dimension_reference(self) -> DimensionReference: # noqa: D
return DimensionReference(element_name=self.element_name)


@dataclass(frozen=True)
@dataclass(frozen=True, order=True)
class MetricReference(ElementReference): # noqa: D
pass

Expand All @@ -66,14 +66,14 @@ class ModelReference(SerializableDataclass):
pass


@dataclass(frozen=True)
@dataclass(frozen=True, order=True)
class SemanticModelReference(ModelReference):
"""A reference to a semantic model definition in the model."""

semantic_model_name: str


@dataclass(frozen=True)
@dataclass(frozen=True, order=True)
class SemanticModelElementReference(ModelReference):
"""A reference to an element definition in a semantic model definition in the model.

Expand Down Expand Up @@ -101,7 +101,7 @@ def is_from(self, ref: SemanticModelReference) -> bool:
return self.semantic_model_name == ref.semantic_model_name


@dataclass(frozen=True)
@dataclass(frozen=True, order=True)
class MetricModelReference(ModelReference):
"""A reference to a metric definition in the model."""

Expand Down
8 changes: 4 additions & 4 deletions tests/parsing/test_metric_parsing.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,8 +55,8 @@ def test_legacy_metric_input_measure_object_parsing() -> None:
measure:
name: legacy_measure_from_object
filter: "{{ dimension('some_bool') }}"
join_to_timespine: true
fill_nulls_with: 1
time_spine_join: true
null_fill_value: 1
"""
)
file = YamlConfigFile(filepath="inline_for_test", contents=yaml_contents)
Expand All @@ -70,8 +70,8 @@ def test_legacy_metric_input_measure_object_parsing() -> None:
filter=PydanticWhereFilterIntersection(
where_filters=[PydanticWhereFilter(where_sql_template="""{{ dimension('some_bool') }}""")]
),
join_to_timespine=True,
fill_nulls_with=1,
time_spine_join=True,
null_fill_value=1,
)


Expand Down
Loading