-
Notifications
You must be signed in to change notification settings - Fork 97
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow measures that join to time spine to use agg_time_dim
#1005
Merged
Merged
Changes from 1 commit
Commits
Show all changes
21 commits
Select commit
Hold shift + click to select a range
89df4b3
Changelog
courtneyholcomb 73f07dc
Rename JoinToTimeSpineNode param from requested_metric_time_dimension…
courtneyholcomb e70f24c
Update dataflow plan
courtneyholcomb ab81fbb
Update dataflow to sql
courtneyholcomb 2cec7be
Rename params from metric_time_dimension -> agg_time_dimension
courtneyholcomb c3d5e0e
Update validations
courtneyholcomb d46672f
Add integration tests
courtneyholcomb c6cbed1
Add query rendering tests
courtneyholcomb c5a7a3b
Update snapshots
courtneyholcomb 6b3b3a8
Update dataflow plan
courtneyholcomb fd61f3c
Add integration test
courtneyholcomb 78f7b44
Changelog
courtneyholcomb 86075fa
Grammar
courtneyholcomb 0b04bb2
Add cleaner methods to find queried agg_time_dimensions
courtneyholcomb ee3c163
Merge main
courtneyholcomb 9710ac8
Move array length check to node
courtneyholcomb 172fa73
Remove unneeded line
courtneyholcomb c80ab67
Add query_includes_metric_time property to JoinToTimeSpineNode
courtneyholcomb f3ade5b
Merge main
courtneyholcomb aa2bead
Use helper to dedupe logic
courtneyholcomb 3c932f9
Merge main
courtneyholcomb File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
Oops, something went wrong.
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 was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, so previously this would have just run the query but skipped the join, correct? That seems strange and incorrect to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logic there was that if there's no time dimension to join to, we can't join to time spine anyway.
And then, if there is a time dimension that isn't agg time, should we time spine join to that dimension? Seems inconsistent if we're treating it like any other categorical dimension in all other ways.