rfc: distributed dynamic filtering - #553
jayshrivastava wants to merge 17 commits into
Conversation
gabotechs
left a comment
There was a problem hiding this comment.
This is looking very good! I really do not have any major comments, this seems to be going in the right direction.
| The proposed change is to implement dynamic filtering similarly to Trino. | ||
|
|
||
| 1. During execution, the dynamic filter producers (`HashJoinExec`, `SortExec`, `AggregateExec`) will send dynamic filter updates to the coordinator. | ||
| 2. The coordinator will union / merge the dynamic filter updates from all workers. | ||
| 3. The coordinator will send the unioned / merged dynamic filter updates to the consumers (`DataSourceExec`). |
There was a problem hiding this comment.
👍 Sounds good, with the current infrastructure, I don't think it would be terrible difficult to do this
| - Less overhead than ORing | ||
|
|
||
| Cons: | ||
| - Is brittle. What if we have to support non-range and non-IN-LIST expressions? It would be nice if dynamic filters in vanilla datafusion natively implemented a `merge` or `union` operation |
There was a problem hiding this comment.
Why do you think it's brittle? For merging, I imagine you can just rely on Arrow comparison kernels for deduping IN LIST expressions.
There was a problem hiding this comment.
Can you expand on this? It's easy to implement a merge operation if we assume the expressions to merge always look like a@0 >= 0 AND a@0 <= 5 OR IN LIST [10, 11]. We would downcast to InListExpr and RangeExpr and define a merge operation for those.
In the future, what if there's other kinds of expressions in there? Then maybe the merge operation would break.
Maybe @LiaCastaneda has more insights on this. I think she implemented something similar.
There was a problem hiding this comment.
I imagine that not any arbitrary expression can be placed there. Even if it can, you still decide to optimize the most common cases, like IN LISTs or min/max ranges, and if there's anything else besides does two things just leave it OR-ed.
There was a problem hiding this comment.
In the future, what if there's other kinds of expressions in there?
I think that rather than adding other kinds of expressions, there is probably a higher possibility that the overall dynamic filter shape changes from the current CASE statement to something else.
Regarding expression types, I think the other discussed expression that hasn’t been implemented yet is bloom filters pushdown as dynamic filters. In that case, deduping probably becomes awkward, I’m not really sure how you would merge that (or if its possible). But I think it’s a very niche use case, and as Gabriel said, it can be ORed anyway.
|
|
||
| // Read Updates | ||
| pub fn current(&self) -> Result<Arc<dyn PhysicalExpr>> | ||
| pub(crate) fn subscribe(&self) -> DynamicFilterSubscription |
There was a problem hiding this comment.
🤔 subscribe not being public might be a problem.
There was a problem hiding this comment.
True. I think we can just use this instead https://github.com/apache/datafusion/blob/3a29d6bd8cc9ac2bf5efee9f070dcdeea9f97b32/datafusion/physical-expr/src/expressions/dynamic_filters/mod.rs?plain=1#L320 since we would wait for completion anyways.
|
|
||
| #### Options 1 & 2: Support new APIs In Vanilla DataFusion | ||
|
|
||
| See https://github.com/apache/datafusion/issues/23814 |
There was a problem hiding this comment.
apache/datafusion#23814 seems to be evolving in a good direction, and people seem engaged in the discussion, so I'd lean towards using whatever comes out of that.
There was a problem hiding this comment.
Yup I will stay on top of that issue.
| For correctness, the coordinator must wait for all dynamic filter updates from all workers before sending them to the consumers, otherwise, the | ||
| consumers may prune rows incorrectly. |
There was a problem hiding this comment.
Is this
A) the coordinator must get an update from each worker at least once before sending the updated expression, and then can update eagerly going forward
B) the coordinator must get an update from each worker at least once per filter generation before sending the next generation?
C) or something else?
Relatedly, this is overly strict for SortExec's TopK dynamic filter, as well as AggregateExec's min/max filter, which can safely be updated eagerly. (I've no problem with correctness then optimization as the sequence for shipping this though)
There was a problem hiding this comment.
Great point. For hash joins, eager updates aren't safe. The filter effectively contains the build side of the join. If the join is on different workers and each has a different build side, we need filter ORed together to get a global filter which represents the entire build side across workers.
However for sort and aggregate, I think eager updates are very safe. For an aggregate that has MIN(a) for example the aggregate on any worker can push down a < 10 for example. We can OR the expressions across workers ex. a < 10 OR a < 20 and the minimum value, 10 would win. This mechanism is similar for SortExec.
For now, I think we should ensure on correctness / safety in the initial implementation, so I'm proposing option A) for now. I think we can roll out eagerness later. This will mean the dynamic filters on SortExec and AggregateExec will still be pretty useless for now, but we will be able to unlock joins.
Since you worked on joins recently, maybe you can confirm if what I said makes sense
There was a problem hiding this comment.
Yeah, after reading up a bit more on HashJoinExec's filter strategy, that makes sense, and A) is a reasonable general approach.
We'll want to consider how we can track each expression's producer node's type, as that will be necessary info for enabling eagerness correctly later.
There was a problem hiding this comment.
We'll want to consider how we can track each expression's producer node's type, as that will be necessary info for enabling eagerness correctly later.
Agreed. At least, maybe some property to indicate if its safe to apply eager updates or not.
I pushed a short section in the RFC on eager updates so this is documented.
…he#22437) (apache#24018) ## Which issue does this PR close? <!-- We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123. --> - Informs: apache#23814 - Informs: datafusion-contrib/datafusion-distributed#584 This change does not close the above issues because it does not implement a way to tell if a node is a producer dynamic filters. ## Rationale for this change See apache#23814 and datafusion-contrib/datafusion-distributed#553. To send dynamic filter updates across the network, there needs to be a way to get access to `PhysicalExpr` from `ExecutionPlan`. As discussed in apache#23814, the cleanest way to do this is to add `ExecutionPlan::apply_expressions`, which mirrors a similar method for logical plan nodes. ## What changes are included in this PR? There's 3 commits in this PR: Firstly, commit 1 re-applies the changes in apache#20337 (reverted in apache#22437). Some of the reasons for why the original PR was reverted include (a) `apply_expressions` is too complicated to implement and there's no concrete need to justify this complexity (b) there was no usage of `apply_expressions` inside this repo To address (a) - justification for adding this method is provided in apache#23814 - commit 2 in this PR adds helper methods `apply_expression_roots` and `apply_no_expressions` which abstract away the `TreeNodeRecursion` complexity from implementors. Now, `apply_expressions` very trivial to implement ex. ```rust fn apply_expressions( &self, f: &mut dyn FnMut(&Arc<dyn PhysicalExpr>) -> Result<TreeNodeRecursion>, ) -> Result<TreeNodeRecursion> { apply_expression_roots([&self.predicate_1], f) apply_expression_roots([&self.predicate_2], f) apply_expression_roots([&self.other_expression], f) } ``` - the method traverses over `&Arc<dyn PhysicalExpr>` rather than `&dyn PhysicalExpr` to reduce complexity around lifetimes To address (b): - commit 3 adds a usage of `apply_expressions` in `physical-plan/src/aggregates/mod.rs`. Previously, there was a hack that checked if a filter was pushed down using `Arc::strong_count(dyn_filter) > 1`. Now it uses `apply_expressions` - similarly, commit 4 removes `is_used` from dynamic filters which used to check Arc references counts to see if a filter was pushed down. Now, the hash join uses `apply_expressions` to find pushed down filters. ## Are these changes tested? Yes. ## Are there any user-facing changes? There's a new mandatory method `ExecutionPlan::apply_expressions()`. See the upgrading guide and documentation for details. --------- Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>
shinzoxD
left a comment
There was a problem hiding this comment.
The new eager-update section leaves one liveness boundary to clarify.
| #### Sorts | ||
| These operate similarly to Aggregates and can be applied eagerly. | ||
|
|
||
| The proposal in this RFC is to take the safest approach and wait for all filters to complete before |
There was a problem hiding this comment.
The preceding paragraph says aggregate filters are never marked complete, so a literal wait for all discovered filters can never finish for a plan containing AggregateExec (and presumably SortExec). Could the initial scope explicitly define the required-completion set as join-produced filters only, and state that filter collection must not block query execution? Otherwise an implementation that registers every producer can either never publish these filters or deadlock while awaiting completion. Non-completing producers can then be added when eager updates land.
…he#22437) (apache#24018) ## Which issue does this PR close? <!-- We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes apache#123` indicates that this PR will close issue apache#123. --> - Informs: apache#23814 - Informs: datafusion-contrib/datafusion-distributed#584 This change does not close the above issues because it does not implement a way to tell if a node is a producer dynamic filters. ## Rationale for this change See apache#23814 and datafusion-contrib/datafusion-distributed#553. To send dynamic filter updates across the network, there needs to be a way to get access to `PhysicalExpr` from `ExecutionPlan`. As discussed in apache#23814, the cleanest way to do this is to add `ExecutionPlan::apply_expressions`, which mirrors a similar method for logical plan nodes. ## What changes are included in this PR? There's 3 commits in this PR: Firstly, commit 1 re-applies the changes in apache#20337 (reverted in apache#22437). Some of the reasons for why the original PR was reverted include (a) `apply_expressions` is too complicated to implement and there's no concrete need to justify this complexity (b) there was no usage of `apply_expressions` inside this repo To address (a) - justification for adding this method is provided in apache#23814 - commit 2 in this PR adds helper methods `apply_expression_roots` and `apply_no_expressions` which abstract away the `TreeNodeRecursion` complexity from implementors. Now, `apply_expressions` very trivial to implement ex. ```rust fn apply_expressions( &self, f: &mut dyn FnMut(&Arc<dyn PhysicalExpr>) -> Result<TreeNodeRecursion>, ) -> Result<TreeNodeRecursion> { apply_expression_roots([&self.predicate_1], f) apply_expression_roots([&self.predicate_2], f) apply_expression_roots([&self.other_expression], f) } ``` - the method traverses over `&Arc<dyn PhysicalExpr>` rather than `&dyn PhysicalExpr` to reduce complexity around lifetimes To address (b): - commit 3 adds a usage of `apply_expressions` in `physical-plan/src/aggregates/mod.rs`. Previously, there was a hack that checked if a filter was pushed down using `Arc::strong_count(dyn_filter) > 1`. Now it uses `apply_expressions` - similarly, commit 4 removes `is_used` from dynamic filters which used to check Arc references counts to see if a filter was pushed down. Now, the hash join uses `apply_expressions` to find pushed down filters. ## Are these changes tested? Yes. ## Are there any user-facing changes? There's a new mandatory method `ExecutionPlan::apply_expressions()`. See the upgrading guide and documentation for details. --------- Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>
78b65f1 to
b8eddac
Compare
300b5af to
e114458
Compare
## Stack This stack of PRs implements distributed dynamic filtering #528 1. #623 <- you are here 2. #634 3. #635 4. #636 5. #637 6. #639 Closes: #529 ## Problem Post df-55 upgrade, dynamic filters should work in the worker-local case. There's no way to observe them working other than looking at metrics. ``` ┌───── Stage 2 ── tasks=1 │ AggregateExec: Final COUNT(*) │ [Stage 1] => NetworkCoalesceExec └────────────────────────────────────────────────── ┌───── Stage 1 ── tasks=2 │ HashJoinExec: orders.customer_id = selected_customers.customer_id │ DistributedLeafExec: | ... │ DistributedLeafExec: │ t0: DataSourceExec: predicate=DynamicFilter [ empty ] │ t1: DataSourceExec: predicate=DynamicFilter [ empty ] └──────────────────────────────────────────────── ``` Ideally we want the final filters visible when displaying plans. ## Solution This PR adds a new protocol which is basically identical to the metrics protocol. Even the `MetricsStore` is now just `Store` and is generic over `TaskMetrics` and `TaskCompletedDynamicFilters` (contains completed dynamic filters for a task). ```rust pub(crate) type MetricsStore = Store<TaskMetrics>; pub(crate) type CompletedDynamicFilterStore = Store<TaskCompletedDynamicFilters>; ``` Similar to the metrics protocol, workers now collect completed dynamic filters and send them back to the coordinator. ``` Coordinator Worker ----------- ------ Create independent display copies | +-- SetPlan(task 0, filter IDs) -------> Decode plan | | | | execute | | | | | | | | | | task finishes | v |<----- TaskDynamicFilters ----- Serialize completed filters from the consumers | v ``` Then, at display time, we call `apply_reports_to_distributed_leaves` which traverses the `plan_for_viz` and updates the dynamic filters for all the variants: ``` DistributedLeafExec task 0: DynamicFilter [ key@0 >= 1 AND key@0 <= 10 ] task 1: DynamicFilter [ empty ] ``` ## Notes ### Duplicate RPC Messages We will eventually have more dynamic filter RPCs which manage the worker -> coordinator -> merge -> worker flow mentioned in #553. In theory, the coordinator will know at `merge` time what the completed filters are, making the `TaskCompletedDynamicFilters` and final worker -> coordinator message in this PR irrelevant. However, I think having these mechanisms be separate is good because a) it helps us validate that the dynamic filter coordinator -> worker flow work using external "oracle", and b) there's no guarantee that the coordinator -> worker propagation happens before the query is done (ex. the `DataSourceExec` may not block execution waiting for dynamic filters), so it's good to have a separate way to know if the final `DataSourceExec` applied a filter or not. ### `AND true` and empty filters ``` DynamicFilter [ sr_returned_date_sk@0 >= 2451545 AND sr_returned_date_sk@0 <= 2451910 AND true ] AND DynamicFilter [ empty ] ``` In this filter `AND true` occurs because of apache/datafusion#24277. The first `DynamicFilter` is active but we lose the `HashTableLookupExpr` when serializing it to send back to the coordinator. The 2nd filter is `DynamicFilter [ empty ]` because this is a dynamic filter produced by a remote producer, which does not get propagated to this node yet. This will be fixed later. ### Displaying Dynamic Filters Protocol is as similar to the metrics protocol as possible. Due to double wrapping (`MetricsWrapperExec` wraps `DistributedLeafExec`, it's tricky to do the dynamic filter rewrite after doing the metrics rewrite. So `rewrite_distributed_plan_with_dynamic_filters` has to be called **first**. ```rust let plan = rewrite_distributed_plan_with_dynamic_filters(plan).await?; let plan = rewrite_distributed_plan_with_metrics(plan, DistributedMetricsFormat::Aggregated).await?; println!("{}", display_plan_ascii(plan.as_ref(), true)); ``` ## Testing - Tests in `tests/dynamic_filtering.rs`
Rendered View: https://github.com/datafusion-contrib/datafusion-distributed/blob/js/dynamic-filtering-design/rfcs/1-distributed-dynamic-filtering.md
Informs #528