Simplify JoinToTimeSpineNode
#1542
Open
+869
−874
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
courtneyholcomb
force-pushed
the
court/simp4
branch
from
5dcb16b
to
52e7d6c
Compare
courtneyholcomb
force-pushed
the
court/simp3
branch
from
81e54f2
to
d4c5482
Compare
courtneyholcomb
force-pushed
the
court/simp4
branch
from
52e7d6c
to
73a8591
Compare
courtneyholcomb
commented
Nov 21, 2024
| @@ -322,54 +323,54 @@ def _make_time_spine_data_set( | |||
| """ | |||
| time_spine_table_alias = self._next_unique_table_alias() | |||
|
|
|||
| agg_time_dimension_specs = {instance.spec for instance in agg_time_dimension_instances} | |||
| queried_specs = {instance.spec for instance in agg_time_dimension_instances} | |||
There was a problem hiding this comment.
Renamed to differentiate more clearly from required_specs.
courtneyholcomb
force-pushed
the
court/simp4
branch
2 times, most recently
from
d0996f3
to
b4e4e9c
Compare
courtneyholcomb
commented
Nov 21, 2024
| @@ -19,8 +19,8 @@ FROM ( | |||
| FROM ( | |||
| -- Join to Time Spine Dataset | |||
| SELECT | |||
| DATE_TRUNC('month', subq_12.ds) AS metric_time__month | |||
| , subq_10.metric_time__day AS metric_time__day | |||
| subq_12.ds AS metric_time__day | |||
There was a problem hiding this comment.
Here you can see we are now pulling the join column from the time spine, where in the last PR we were not.
It is no longer used.
The biggest change in this commit is to remove all logic related to building select columns from visit_join_to_time_spine_node(). This logic was duplicated in _make_time_spine_data_set(). That logic is now consolidated to _make_time_spine_data_set(). This is laying the foundation for a change coming up the stack, which will replace _make_time_spine_data_set() with its own node in the DataflowPlan. There is also some other cleanup in this commit because to make visit_join_to_time_spine_node() simpler and more readable.
This sorting was not commprehensive and resulted in inconsistent snapshots. Removed the custom sorting here and retained the original order, instead.
courtneyholcomb
force-pushed
the
court/simp4
branch
from
b4e4e9c
to
0781ab3
Compare
courtneyholcomb
added
the
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
A bunch of cleanup in the dataflow to SQL logic for the
JoinToTimeSpineNode. There was a lot of duplication and hard to read code here.Notes: