Support multiple time spines in one query #1644
Merged
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
commented
Jan 28, 2025
| 2020-01-08T00:00:00 2020-01-01T10:00:00 1 3 | ||
| 2020-01-08T00:00:00 2020-01-01T11:00:00 1 3 | ||
| 2020-01-08T00:00:00 2020-01-01T12:00:00 1 3 | ||
| 2020-01-08T00:00:00 2020-01-02T01:00:00 None None |
There was a problem hiding this comment.
These nulls are expected due to the the join_to_timespine metric. They span all the rows that we have in our hourly test time spine.
There was a problem hiding this comment.
Note: I'm planning to make some improvements to our time spine test data up the stack. Namely:
- Some of the feedback you gave me previously about
martian_day - Adding at least one more custom granularity (something fairly standard like
fiscal_quarter) - Adding more test data to our hourly time spine so that it at least spans more than one custom granularity period
courtneyholcomb
commented
Jan 29, 2025
| raise RuntimeError( | ||
| str( | ||
| LazyFormat( | ||
| "Unexpected number of time spine sources required for query - cumulative metrics should require exactly one.", |
There was a problem hiding this comment.
Custom grains are joined later for cumulative metrics, so this should always be true.
courtneyholcomb
force-pushed
the
court/custom-offset8.5
branch
from
026c56f to
fde848b
Compare
courtneyholcomb
force-pushed
the
court/custom-offset8
branch
from
dbeded9 to
e018ee3
Compare
courtneyholcomb
force-pushed
the
court/custom-offset8.5
branch
from
fde848b to
06958f2
Compare
courtneyholcomb
force-pushed
the
court/custom-offset8.5
branch
from
06958f2 to
a736cee
Compare
plypaul
approved these changes
Feb 4, 2025
There was a problem hiding this comment.
👍 Looking forward to the update on the test dataset as you mentioned in 1e56f3b#r1933150079
plypaul
approved these changes
Feb 4, 2025
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.
There are some scenarios where a user might require multiple time spines per query:
1 is already supported. This PR provides support for 2.
Not covered: option 2 with a custom offset window metric. This is coming in the next PR up the stack.