-
Notifications
You must be signed in to change notification settings - Fork 39
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
chore(weave): Implement Predicate Pushdown for Calls #1712
Conversation
Preview this PR with FeatureBee: https://beta.wandb.ai/?betaVersion=8a4cc50c59f174dbbeb41042ef1b2854cae58157 |
Preview this PR with FeatureBee: https://beta.wandb.ai/?betaVersion=fdf736c117ccf44e56957ebfa85abd873d201011 |
2 similar comments
Preview this PR with FeatureBee: https://beta.wandb.ai/?betaVersion=fdf736c117ccf44e56957ebfa85abd873d201011 |
Preview this PR with FeatureBee: https://beta.wandb.ai/?betaVersion=fdf736c117ccf44e56957ebfa85abd873d201011 |
proj_params = {"project_id": req.project_id} | ||
|
||
# get all parents | ||
parents = self._select_calls_query( | ||
req.project_id, | ||
conditions=[proj_cond, "id IN {ids: Array(String)}"], | ||
start_event_conditions=[ |
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.
These changes should make deletion a bit faster
conditions_part = _combine_conditions(conditions, "AND") | ||
having_conditions_part = _combine_conditions(having_conditions, "AND") | ||
|
||
where_conditions_part = _make_calls_where_condition_from_event_conditions( |
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.
Here is one of the new clauses
conditions_part = _combine_conditions(conditions, "AND") | ||
having_conditions_part = _combine_conditions(having_conditions, "AND") | ||
|
||
where_conditions_part = _make_calls_where_condition_from_event_conditions( |
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.
Another new clause
def _process_calls_filter_to_conditions( | ||
filter: tsi._CallsFilter, | ||
param_builder: typing.Optional[ParamBuilder] = None, | ||
) -> tuple[list[str], set[str]]: | ||
) -> FilterToConditions: |
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.
This method has been refactored to return specific conditions where possible, using correct namespacing
@@ -1898,3 +1952,31 @@ def process_operand(operand: tsi_query.Operand) -> str: | |||
conditions.append(filter_cond) | |||
|
|||
return conditions, raw_fields_used | |||
|
|||
|
|||
def _make_calls_where_condition_from_event_conditions( |
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.
create an aggregate filter
This PR implements predicate pushdown for the primary filter clause (not yet implemented for the dynamic mongo queries). It was raised that our filter conditions are being applied after grouping, which is certainly not as efficient as applying the filter pre-grouped (since applying after grouping requires operating on far more rows than needed). In this PR we do the following:
conditions
tohaving_conditions
to specifically mean the conditions applied after the group by (in thehaving
clause of the query)start_event_conditions
andend_event_conditions
which can be used to apply filters before the aggregation. If these conditions are supplied, an inner query is executed which generates anid IN (...)
condition that is applied in theWHERE
clause.Testing: