Build DataflowPlan for custom offset window #1584
Open
+694
−598
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/custom-offset6
branch
from
2719954 to
2dcf124
Compare
courtneyholcomb
force-pushed
the
court/custom-offset6
branch
from
db647f3 to
e91fd30
Compare
courtneyholcomb
force-pushed
the
court/custom-offset5
branch
from
57ba604 to
71cef04
Compare
courtneyholcomb
force-pushed
the
court/custom-offset6
branch
from
e91fd30 to
7e13cf3
Compare
courtneyholcomb
force-pushed
the
court/custom-offset5
branch
2 times, most recently
from
6e642e4 to
a9dc916
Compare
courtneyholcomb
force-pushed
the
court/custom-offset6
branch
2 times, most recently
from
d0c473a to
bebe99d
Compare
courtneyholcomb
force-pushed
the
court/custom-offset5
branch
from
a9dc916 to
7de2bd3
Compare
courtneyholcomb
force-pushed
the
court/custom-offset6
branch
from
bebe99d to
81ae325
Compare
Note that we will also implement a simpler dataflow plan if the user queries the metric with only the granularity used in the offset window. This might not be done before the initial feature release, but the results should be the same. Only differences in performance and readability.
Should only have changes to column order. Only unoptimized queries are impacted.
courtneyholcomb
force-pushed
the
court/custom-offset6
branch
from
3740d81 to
ef1dbdc
Compare
plypaul
approved these changes
Dec 19, 2024
| def _offset_window_is_custom(self, offset_window: Optional[MetricTimeWindow]) -> bool: | ||
| return ( | ||
| offset_window is not None | ||
| and offset_window.granularity in self._semantic_model_lookup.custom_granularity_names |
There was a problem hiding this comment.
What happens when the granularity is not in custom_granularity_names? Is this handled through a validation?
| output_node = JoinToTimeSpineNode.create( | ||
| metric_source_node=output_node, | ||
| time_spine_node=time_spine_node, | ||
| requested_agg_time_dimension_specs=queried_agg_time_dimension_specs, | ||
| join_on_time_dimension_spec=self._sort_by_base_granularity(queried_agg_time_dimension_specs)[0], | ||
| offset_window=metric_spec.offset_window, | ||
| offset_window=( |
There was a problem hiding this comment.
Are there cases where you can use a JoinToTimeSpineNode with a custom offset window? If not, an assertion in the class would help.
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.
This is the dataflow plan that will be used if the custom grain is queried with any grains that aren't the same as the grain used in the offset window. There will be a simpler dataflow plan used for queries using only the same grain as what's used in the offset window.