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.
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
Support for
Dimension.grain(...)
in where/filter #152Support for
Dimension.grain(...)
in where/filter #152Changes from 1 commit
7faca70
1aff3c9
29eeceb
23edab7
1c7b68a
584ef92
3835584
5309a49
bcc6d8e
e3ef46c
3d7f5fe
13ebd52
804937a
a374894
31a1bb0
ef68e74
03dc547
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Let me know if I'm getting out pocket on this 🙃 I'm likely biased from my personal preference for functional programming, but I found chain of passing the pointer for
time_dimension_factory.time_dimension_call_parameter_sets
odd. Additionally, until I followed the chain I was confused as to why in thewhere_filter_parser
dimension_parameter_sets were getting skipped unless they didn't have atime_granularity
specified. That is to say it appeared that the dimension_parameter_sets withtime_granularity
specified were getting trashed. I understand how it works now, and it's elegant in an OOP way. I do worry it's more complex than it needs to be though and that it will be harder for those unfamiliar with the code to approach. Additionally, thinking more about it, a question in my mind came up of what would happen if someone did the followingThis chain wouldn't break on parsing and it would add the dimension to the
time_dimension_call_parameter_sets
twice, once with granularityDAY
and once with granularityWEEK
. Maybe that is what we want, but I don't think we do? My assumption here is that we'd actually just want it added once with granularityWEEK
.A way to simplify things and to solve the duplication problem would be to not have
WhereFilterDimension
s directly modify thetime _dimension_call_parameter_sets
. Instead whengrain
is called it could just generate aTimeDimensionCallParameterSet
and store it on a self property (maybe something likeself.time_dimension_call_parameter_set
which is defaulted toNone
on instantiation of theWhereFilterDimsnion
. Then inparse_call_parameter_sets
when we iterate overdimension_factory.created
we'd separate the created parameter sets into two lists, one ofDimensionCallParameterSet
s and the other ofTimeDimensionCallParameterSet
. The latter of which would be joined to thetime_dimension_factory.created
in the return statement.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.
I will add comments to this section and look for other opportunities to add comments.
Good call. I hadn't considered that edge case.
Good idea. I had implemented something similar to this, but I wasn't sure if the order of
time_dimension_call_parameter_sets
mattered, so I refactored to this current approach. If we did what you are suggesting, then theTimeDimensionCallParameterSet
s that are generated from this syntaxDimension('metric_time').grain('day')
would always be appended after the ones that are generated from theTimeDimension
syntax. Paul mentioned that the order doesn't matter (but I had already completed the refactor), so I think I will go ahead and implement what you have suggested.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.
Yeah +1 to @QMalcolm here, looking forward to the update!
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.
I believe I have addressed all of your concerns. Let me know if you would prefer a different approach.
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.
Will this reference passing and hidden update extensions be addressed by the approach @QMalcolm suggests? I think it will. If so that's pretty nice, because this is confusing - it looks as if we skip any time dimensions defined through the Dimension() syntax, but that isn't the case.
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.
I think I have fixed your concern, but please let me know if you prefer a different approach. I don't think there is a way to implement this without the mutable
WhereFilterTimeDimensionFactory.time_dimension_call_parameter_sets
,WhereFilterEntityFactory.entity_call_parameter_sets
, &WhereFilterDimensionFactory.created
properties given the protocols that we want to implement.This file was deleted.