Simplify dataflow to SQL logic for JoinOverTimeRangeNode
#1540
Conversation
There should be no functional changes in this commit, only cleanup and readability improvements. Mostly involves moving complex logic to helper functions.
|
|
||
|
|
||
| @dataclass(frozen=True) | ||
| class AnnotatedSqlDataSet: |
There was a problem hiding this comment.
This class should be unchanged. I needed to move it to resolve circular imports.
| @@ -1390,6 +1361,21 @@ def visit_semi_additive_join_node(self, node: SemiAdditiveJoinNode) -> SqlDataSe | |||
| ), | |||
| ) | |||
|
|
|||
| def _choose_instance_for_time_spine_join( | |||
There was a problem hiding this comment.
This helper function is only used once at this point, but it will be used again for the JoinToTimeSpineNode farther up the stack.
| @@ -803,6 +803,7 @@ def transform(self, instance_set: InstanceSet) -> SelectColumnSet: # noqa: D102 | |||
| ) | |||
|
|
|||
|
|
|||
| # TODO: delete this class & all uses. It doesn't do anything. | |||
There was a problem hiding this comment.
This class is supposed to change the column names, but if the specs didn't change, then the column names shouldn't either, so it seems like it doesn't do anything. LMK if I'm overlooking something here!
There was a problem hiding this comment.
This used to be needed but maybe things have changed. There were cases where nodes did not output data sets where the column name in the generated SQL did not match the defined format. Trying to remember what that was though, but there was a bug fix that required this transform.
| "No appropriate agg_time_dimension was found to join to the time spine. " | ||
| "This indicates that the dataflow plan was configured incorrectly." | ||
| ) | ||
| agg_time_dimension_instances.sort(key=lambda instance: instance.spec.time_granularity.base_granularity.to_int()) |
There was a problem hiding this comment.
Not related to this PR, but we probably should just make the TimeGranularity enum orderable in DSI at some point since this operation has come up a few times.
| @@ -803,6 +803,7 @@ def transform(self, instance_set: InstanceSet) -> SelectColumnSet: # noqa: D102 | |||
| ) | |||
|
|
|||
|
|
|||
| # TODO: delete this class & all uses. It doesn't do anything. | |||
There was a problem hiding this comment.
This used to be needed but maybe things have changed. There were cases where nodes did not output data sets where the column name in the generated SQL did not match the defined format. Trying to remember what that was though, but there was a bug fix that required this transform.
There should be no functional changes in this commit, only cleanup and readability improvements. Mostly involves moving complex logic to helper functions.