JoinToTimeSpineNode bug fix
#1541
Conversation
courtneyholcomb
force-pushed
the
court/simp3
branch
from
1de52ca to
5dcb16b
Compare
courtneyholcomb
changed the title
JoinToTimeSpineNodeJoinToTimeSpineNode bug fix
courtneyholcomb
force-pushed
the
court/simp3
branch
from
5dcb16b to
81e54f2
Compare
| @@ -646,9 +646,6 @@ def _build_derived_metric_output_node( | |||
| queried_agg_time_dimension_specs = queried_linkable_specs.included_agg_time_dimension_specs_for_metric( | |||
| metric_reference=metric_spec.reference, metric_lookup=self._metric_lookup | |||
| ) | |||
| assert ( | |||
There was a problem hiding this comment.
This error is handled in the __post_init__ for JoinToTimeSpineNode, so I removed the duplication.
| ): | ||
| agg_time_dimension_instances.append(instance) | ||
| # Select the dimension for the join from the parent node because it may not have been included in the request. | ||
| # Default to using metric_time for the join if it was requested, otherwise use the agg_time_dimension. |
There was a problem hiding this comment.
This is the one place we still have some differentiation between metric_time and agg_time. If you request both, we have to choose one of them to use as the join column, and we default to using metric_time.
| subq_12.ds AS metric_time__day | ||
| , DATE_TRUNC('month', subq_12.ds) AS metric_time__month | ||
| DATE_TRUNC('month', subq_12.ds) AS metric_time__month | ||
| , subq_10.metric_time__day AS metric_time__day |
There was a problem hiding this comment.
You might notice on this line we're selecting metric_time__day from the parent instead of the time spine. This is only true for columns used in the join and never anywhere else, and it is fine because the column should be equivalent to the time spine column since it's the one used in the join.
This is true in a couple of snapshots. It's not a problem functionally but might be confusing to some users reading the SQL. This gets fixed farther up the stack.
There was a problem hiding this comment.
Example of the fix in the next PR here: c6d6323#r1852366470
courtneyholcomb
force-pushed
the
court/simp3
branch
from
81e54f2 to
d4c5482
Compare
courtneyholcomb
added
Run Tests With Other SQL Engines
courtneyholcomb
temporarily deployed
to
DW_INTEGRATION_TESTS
GitHub Actions
Inactive
courtneyholcomb
temporarily deployed
to
DW_INTEGRATION_TESTS
GitHub Actions
Inactive
courtneyholcomb
temporarily deployed
to
DW_INTEGRATION_TESTS
GitHub Actions
Inactive
courtneyholcomb
temporarily deployed
to
DW_INTEGRATION_TESTS
GitHub Actions
Inactive
github-actions
bot
removed
the
Run Tests With Other SQL Engines
| @@ -244,3 +244,16 @@ def with_entity_prefix(self, entity_prefix: EntityReference) -> TimeDimensionSpe | |||
| date_part=self.date_part, | |||
| aggregation_state=self.aggregation_state, | |||
| ) | |||
|
|
|||
| @staticmethod | |||
| def with_base_grains(time_dimension_specs: Sequence[TimeDimensionSpec]) -> List[TimeDimensionSpec]: | |||
There was a problem hiding this comment.
General preference has been to return immutable types, or a more abstract collection type. Similar to https://docs.python.org/3/library/typing.html#typing.List
| @@ -145,6 +145,12 @@ def with_entity_prefix( | |||
| spec=transformed_spec, | |||
| ) | |||
|
|
|||
| def with_new_defined_from(self, defined_from: Tuple[SemanticModelElementReference, ...]) -> TimeDimensionInstance: | |||
courtneyholcomb
force-pushed
the
court/simp2
branch
from
e1a4bab to
5986a03
Compare
…grains This does not change any query behavior, but will allow us to remove this logic from the SQL conversion logic, which is important for future commits.
…ed in JoinToTimeSpineNode
These snapshot changes primarily show changes from the bug fix mentioned in the last commit. The optimized snapshots show the actual behavior change.
courtneyholcomb
force-pushed
the
court/simp3
branch
from
657993f to
3c7b851
Compare
We had previously made a product decision about the behavior of the
JoinToTimeSpineNodethat we later decided was not correct. This is specifically related to what happens if you query with bothmetric_timeand theagg_time_dimension(e.g.,booking__ds). If you query with just one or the other, we would use the time spine to fulfill that dimension. But if you queried with both, there was different behavior.Previous behavior when querying with both:
metric_timeas the time spine dimension and fulfill those values with a time spine columnagg_time_dimensionas any other dimension, and fulfill those values from the semantic model source tableThis behavior felt inconsistent, since you might get different values when querying the
agg_time_dimensiondepending on if you includedmetric_timein the query or not. This could be especially confusing if you includedagg_time_dimensiononly in the group by, andmetric_timein the where filter.New behavior when querying with both:
metric_timeandagg_time_dimensionthe same. Fulfill both dimensions with values from the time spine.PR notes: