-
Notifications
You must be signed in to change notification settings - Fork 99
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
Encapsulate time range constraints for predicate pushdown #1215
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
Thank you for your pull request! We could not find a changelog entry for this change. For details on how to document a change, see the contributing guide. |
876b8fd
to
5ee3296
Compare
10a9d01
to
bfb75ba
Compare
@@ -59,6 +59,16 @@ class MultiHopJoinCandidate: | |||
lineage: MultiHopJoinCandidateLineage | |||
|
|||
|
|||
@dataclass(frozen=True) | |||
class PredicatePushdownParameters: |
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.
Nit: The issue that I've run into with classes that are plural is that when you have a variable that is a collection of them, it's unclear how to name the variable. I've been using ...ParameterSet
instead, but up for other ideas.
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.
Oh yeah, good point. I don't think we'd ever want to collect these but also I never like having the name parameters in a class. It's not utils-level bad but it still basically means "bag of crap."
Maybe PredicatePushdownState? It effectively tracks the known predicates and whether or not to attempt to push them down, so at any point you have a representation of state you can use to apply a constraint (or not).
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.
Oops I already use PredicatePushdownState as a state enumeration later on. 🤦
PredicatePushdownDirective
? PredicatePushdownEvaluationInput
? ThingsWeNeedToDoPredicatePushdownCorrectly
?
I'm going with directive because it's least verbose. We can rename later if we need to.
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.
5ee3296
to
7aa7f5e
Compare
bfb75ba
to
55ed28e
Compare
7aa7f5e
to
fc69e24
Compare
55ed28e
to
88723e2
Compare
fc69e24
to
820afa1
Compare
88723e2
to
ece1e07
Compare
MetricFlow currently allows for a limited scope form of filter predicate pushdown that is particular to time range constraints where the querying user has provided an explicit time window for us to query against. The application of the pushdown operation is managed by threading the time range constraint through the entire dataflow plan builder and applying the filter operation as appropriate. This is exactly what we need to do for robust predicate pushdown evaluation for our expanded set of pushdown operations. Rather than wire a whole new set of parameters through, we simply encapsulate the time range constraints inside of a new object that is more readily extensible to other predicate pushdown handling. This specific change is as mechanical as possible in order to minimize confusion. Places that stood out for improvement via the encapsulating object have been marked for later updates, which will follow shortly.
ece1e07
to
7e723bc
Compare
MetricFlow currently allows for a limited scope form of filter
predicate pushdown that is particular to time range constraints where
the querying user has provided an explicit time window for us to
query against. The application of the pushdown operation is managed
by threading the time range constraint through the entire dataflow
plan builder and applying the filter operation as appropriate.
This is exactly what we need to do for robust predicate pushdown
evaluation for our expanded set of pushdown operations. Rather than
wire a whole new set of parameters through, we simply encapsulate
the time range constraints inside of a new object that is more
readily extensible to other predicate pushdown handling.
This specific change is as mechanical as possible in order to minimize
confusion. Places that stood out for improvement via the encapsulating
object have been marked for later updates, which will follow shortly.