Repository navigation
worker: apply merged dynamic filters during execution #639
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
Merged
Merged
Changes from all commits
Commits
Show all changes
12 commits
Select commit
Hold shift + click to select a range
520e39b
feat: apply merged dynamic filter updates on worker consumers
jayshrivastava 47b5f82
fix: send producer schema with dynamic filters
jayshrivastava 3f4c23f
pr comments round 1
jayshrivastava 2e70c56
pr comments
jayshrivastava b090e56
reduce dynamic filter display size
jayshrivastava 0b3a283
cleanup proto encoding
jayshrivastava 5dca691
fmt and clippy
jayshrivastava 1059c03
Update src/dynamic_filtering/mod.rs
jayshrivastava 9bed440
Update src/dynamic_filtering/mod.rs
jayshrivastava 15cb572
move code around
jayshrivastava 5035594
wip
jayshrivastava 6d03f14
fmt
jayshrivastava 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 hidden or 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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,7 +1,8 @@ | ||
| use crate::dynamic_filtering::discover_dynamic_filter_consumers; | ||
| use crate::dynamic_filtering::{discover_dynamic_filter_consumers, dynamic_filter_producer_schema}; | ||
| use crate::{ | ||
| ApplyDynamicFilter, CoordinatorToWorkerMsg, MaybeEncoded, ProducedDynamicFilter, TaskKey, | ||
| }; | ||
| use datafusion::arrow::datatypes::SchemaRef; | ||
| use datafusion::common::tree_node::{TreeNode, TreeNodeRecursion}; | ||
| use datafusion::common::{HashMap, HashSet, Result, exec_err, internal_err}; | ||
| use datafusion::execution::TaskContext; | ||
|
|
@@ -22,29 +23,40 @@ use tokio::sync::mpsc::UnboundedSender; | |
| use tokio_util::sync::CancellationToken; | ||
|
|
||
| #[derive(Clone, Copy, Debug, PartialEq, Eq)] | ||
| pub(super) enum DynamicFilterMergeMode { | ||
| enum DynamicFilterMergePolicy { | ||
| /// Wait for every planned producer task to report a complete dynamic filter, then | ||
| /// merge and forward the filter. Used for partitioned joins. | ||
| AllProducersComplete, | ||
| /// union and forward the filter. Used for partitioned joins. | ||
| UnionWhenAllComplete, | ||
| /// Wait for any producer to report a complete dynamic filter and forward it. | ||
| /// Used for collect left joins. | ||
| FirstProducerComplete, | ||
| /// Forward any update received from any producer, merging updates from | ||
| /// different producers. Used for TopK dynamic filters in aggregates and sorts. | ||
| Incremental, | ||
| ForwardFirstComplete, | ||
| /// Intersect all updates from producers and immediately forward them. Used | ||
| /// for TopK and Min/Max aggregate dynamic filters where intersecting updates | ||
| /// is safe. | ||
| IntersectOnUpdate, | ||
| } | ||
|
|
||
| #[derive(Default)] | ||
| pub(super) struct PlannedDynamicFilter { | ||
| pub(super) merge_mode: Option<DynamicFilterMergeMode>, | ||
| // Producer and consumer tasks for a dynamic filter. | ||
| // | ||
| // Note that it is not guaranteed that every task within a stage produces / consumes dynamic filters. For | ||
| // example, a distributed union may prevent a dynamic filter from appearing in all tasks. So, we | ||
| // store task keys rather than stage ids. | ||
| // | ||
| /// Registered producers and their latest accepted snapshots, if any. | ||
| pub(super) producers: HashMap<TaskKey, Option<PhysicalDynamicFilterNode>>, | ||
| pub(super) consumer_tasks: HashSet<TaskKey>, | ||
|
|
||
| /// Populated upon task registration when a producer is found. | ||
| /// | ||
| /// The behavior to take when an update arrives from a producer. | ||
| merge_mode: Option<DynamicFilterMergePolicy>, | ||
| /// Schema of the original producer arguments, shared by all producer tasks. | ||
| pub(super) producer_schema: Option<SchemaRef>, | ||
|
|
||
| /// Populated when an update is ready to be sent to consumers. | ||
| /// | ||
| /// Full dynamic filter containing the merged predicate and its completion state. | ||
| pub(super) merged: Option<PhysicalDynamicFilterNode>, | ||
| /// Immutable snapshot shared by local and remote delivery, encoded once per change. | ||
|
|
@@ -110,11 +122,11 @@ impl DynamicFilterRegistry { | |
| .downcast_ref::<HashJoinExec>() | ||
| .is_some_and(|join| matches!(join.partition_mode(), PartitionMode::CollectLeft)) | ||
| { | ||
| DynamicFilterMergeMode::FirstProducerComplete | ||
| DynamicFilterMergePolicy::ForwardFirstComplete | ||
| } else if node.is::<SortExec>() || node.is::<AggregateExec>() { | ||
| DynamicFilterMergeMode::Incremental | ||
| DynamicFilterMergePolicy::IntersectOnUpdate | ||
| } else { | ||
| DynamicFilterMergeMode::AllProducersComplete | ||
| DynamicFilterMergePolicy::UnionWhenAllComplete | ||
| }; | ||
| let produced_ids: HashSet<_> = node | ||
| .dynamic_expressions_produced() | ||
|
|
@@ -131,16 +143,29 @@ impl DynamicFilterRegistry { | |
| } | ||
| }) | ||
| .collect::<Result<_>>()?; | ||
| producers.extend(produced_ids.iter().map(|id| (*id, merge_mode))); | ||
| if !produced_ids.is_empty() { | ||
| let schema = dynamic_filter_producer_schema(node.as_ref())?; | ||
| producers.extend( | ||
| produced_ids | ||
| .iter() | ||
| .map(|id| (*id, merge_mode, Arc::clone(&schema))), | ||
| ); | ||
| } | ||
| Ok(TreeNodeRecursion::Continue) | ||
| })?; | ||
| // We can safely ignore anchors because they are not evaluated by network boundaries. This | ||
| // means they do not need updates forwarded to them. | ||
| let consumers = discover_dynamic_filter_consumers(task_specialized_plan)?.consumers; | ||
|
|
||
| let mut state = self.state.lock().expect("dynamic filter registry poisoned"); | ||
| for (id, merge_mode) in producers { | ||
| for (id, merge_mode, producer_schema) in producers { | ||
| let filter = state.filters.entry(id).or_default(); | ||
| if let Some(existing) = &filter.producer_schema | ||
| && existing != &producer_schema | ||
| { | ||
| return internal_err!("Dynamic filter {id} has conflicting producer schemas"); | ||
| } | ||
| filter.producer_schema.get_or_insert(producer_schema); | ||
| filter.merge_mode = Some(match filter.merge_mode { | ||
| Some(existing) if existing != merge_mode => { | ||
| return internal_err!( | ||
|
|
@@ -234,7 +259,7 @@ impl DynamicFilterRegistry { | |
| report.expression_id | ||
| ); | ||
| }; | ||
| if (filter.merge_mode != Some(DynamicFilterMergeMode::Incremental) | ||
| if (filter.merge_mode != Some(DynamicFilterMergePolicy::IntersectOnUpdate) | ||
| && !dynamic_filter.is_complete) | ||
| || previous | ||
| .as_ref() | ||
|
|
@@ -267,7 +292,7 @@ impl DynamicFilterRegistry { | |
| state.sealed_stages.contains(&task.stage_id) | ||
| && report.as_ref().is_some_and(|report| report.is_complete) | ||
| }); | ||
| if mode == DynamicFilterMergeMode::AllProducersComplete && !all_complete { | ||
| if mode == DynamicFilterMergePolicy::UnionWhenAllComplete && !all_complete { | ||
| return false; | ||
| } | ||
|
|
||
|
|
@@ -277,7 +302,7 @@ impl DynamicFilterRegistry { | |
| .filter_map(|(task, report)| report.as_ref().map(|report| (task, report))) | ||
| .collect(); | ||
| reports.sort_unstable_by_key(|(key, _)| (key.stage_id, key.task_number)); | ||
| if mode == DynamicFilterMergeMode::FirstProducerComplete { | ||
| if mode == DynamicFilterMergePolicy::ForwardFirstComplete { | ||
| reports.truncate(1); | ||
| } | ||
| let Some((_, template)) = reports.first() else { | ||
|
|
@@ -288,9 +313,10 @@ impl DynamicFilterRegistry { | |
| .iter() | ||
| .filter_map(|(_, report)| report.inner_expr.as_deref().cloned()) | ||
| .collect(), | ||
| mode, | ||
| ) | ||
| .map(Box::new); | ||
| let is_complete = mode == DynamicFilterMergeMode::FirstProducerComplete || all_complete; | ||
| let is_complete = mode == DynamicFilterMergePolicy::ForwardFirstComplete || all_complete; | ||
| if previous.is_some_and(|previous| { | ||
| previous.inner_expr == inner_expr && previous.is_complete == is_complete | ||
| }) { | ||
|
|
@@ -328,6 +354,11 @@ impl DynamicFilterRegistry { | |
| let Some(expression) = &filter.merged_bytes else { | ||
| return Ok(()); | ||
| }; | ||
| let Some(producer_schema) = &filter.producer_schema else { | ||
| return internal_err!( | ||
| "Dynamic filter {id} has a merged predicate but no producer schema" | ||
| ); | ||
| }; | ||
| for &task_key in &filter.consumer_tasks { | ||
| // A task-local consumer is already updated directly by its producer. | ||
| if filter.producers.contains_key(&task_key) || state.delivered.contains(&(id, task_key)) | ||
|
|
@@ -340,6 +371,7 @@ impl DynamicFilterRegistry { | |
| let update = CoordinatorToWorkerMsg::ApplyDynamicFilter(Box::new(ApplyDynamicFilter { | ||
| expression_id: id, | ||
| expression: MaybeEncoded::Encoded(expression.clone()), | ||
| producer_schema: Arc::clone(producer_schema), | ||
| })); | ||
| if sender.send(update).is_err() { | ||
| // Closing the channel is expected only once query shutdown has started. | ||
|
|
@@ -356,8 +388,12 @@ impl DynamicFilterRegistry { | |
| } | ||
| } | ||
|
|
||
| /// Merges [`PhysicalExprNode`] together by ORing them. | ||
| fn merge_predicates(mut predicates: Vec<PhysicalExprNode>) -> Option<PhysicalExprNode> { | ||
| /// Merges [`PhysicalExprNode`] together. Merge behavior (ex. AND vs OR) is determined | ||
| /// by the provided [`DynamicFilterMergePolicy`]. | ||
| fn merge_predicates( | ||
| mut predicates: Vec<PhysicalExprNode>, | ||
| mode: DynamicFilterMergePolicy, | ||
| ) -> Option<PhysicalExprNode> { | ||
| match predicates.len() { | ||
| 0 => None, | ||
| 1 => predicates.pop(), | ||
|
|
@@ -366,7 +402,12 @@ fn merge_predicates(mut predicates: Vec<PhysicalExprNode>) -> Option<PhysicalExp | |
| expr_type: Some(ExprType::BinaryExpr(Box::new(PhysicalBinaryExprNode { | ||
| l: None, | ||
| r: None, | ||
| op: "Or".to_owned(), | ||
| op: if mode == DynamicFilterMergePolicy::IntersectOnUpdate { | ||
| "And" | ||
| } else { | ||
| "Or" | ||
| } | ||
| .to_owned(), | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sneaking this change into this PR... :) For sorts and aggregates, we can safely AND. |
||
| operands: predicates, | ||
| }))), | ||
| }), | ||
|
|
||
This file contains hidden or 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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,91 @@ | ||
| use crate::MaybeEncoded; | ||
| use crate::codec::decode_physical_expr; | ||
| use datafusion::arrow::datatypes::Schema; | ||
| use datafusion::common::{Result, internal_err}; | ||
| use datafusion::execution::TaskContext; | ||
| use datafusion::physical_expr::PhysicalExpr; | ||
| use datafusion::physical_expr::expressions::DynamicFilterPhysicalExpr; | ||
| use datafusion_proto::protobuf::physical_expr_node::ExprType; | ||
| use std::sync::Arc; | ||
|
|
||
| /// Applies a `source` dynamic filter expression to a `target` dynamic filter expression. | ||
| /// | ||
| /// This function always uses the original children of the dynamic filter expression | ||
| /// when applying an update. See below. | ||
| /// | ||
| /// ## Normal Behavior | ||
| /// | ||
| /// ```text | ||
| /// HashJoinExec: on=[build.key = probe.key] | ||
| /// │ └── producer: original=[key], children=[key] ──────────────┐ | ||
| /// ├── build │ | ||
| /// └── UnionExec: probe │ | ||
| /// ├── DataSourceExec: phone_number AS key │ shared inner | ||
| /// │ └── consumer 1: original=[key], children=[phone_number] ─┤ | ||
| /// └── DataSourceExec: telephone AS key │ | ||
| /// └── consumer 2: original=[key], children=[telephone] ───┘ | ||
| /// ``` | ||
| /// | ||
| /// 1. [`DynamicFilterPhysicalExpr::update()`] remaps any occurrences of `original` to `children` | ||
| /// and stores the result in the shared state. | ||
| /// | ||
| /// 2. [`DynamicFilterPhysicalExpr::current()`] reads the remapped expression and remaps it again, | ||
| /// mapping `original` to `children` and returns it without storing. | ||
| /// | ||
| /// Example: the producer calls update(`key > 123`) which trivially maps to `key > 123` | ||
| /// and is stored. Then, the consumers call current(), reading from the shared inner state, | ||
| /// to get the remapped `phone_number > 123` and `telephone > 123` expressions respectively. | ||
| /// | ||
| /// ## Failure Mode | ||
| /// | ||
| /// The source expression may be a consumer and cause remapping to fail. | ||
| /// | ||
| /// Say that the source and target are both `consumer 1: original=[key], children=[phone_number]`. | ||
| /// The source is a populated filter from a worker `phone_number > 123` and the target is empty, | ||
| /// used for display. | ||
| /// | ||
| /// In this situation, we should use the inner expression `key > 123` from the source to call | ||
| /// update(`key > 123`). We should not call `update(source.current())` or | ||
| /// `update(phone_number > 123)`, or else the update fails. | ||
| pub(crate) fn apply_dynamic_filter_update( | ||
| target: &Arc<DynamicFilterPhysicalExpr>, | ||
| source: &MaybeEncoded<Arc<dyn PhysicalExpr>>, | ||
| source_schema: &Schema, | ||
| task_ctx: &TaskContext, | ||
| ) -> Result<()> { | ||
| // We need two private fields from the source, so we convert to the proto | ||
| // representation as a workaround. | ||
| // 1. `is_complete` (which is public in df56) | ||
| // 2. The inner expression which has unremapped children (this is only | ||
| // required when the source is another consumer) | ||
| // | ||
| // TODO: Avoid needing to encode to proto: | ||
| // https://github.com/datafusion-contrib/datafusion-distributed/issues/768/ | ||
| let source_proto = source.to_proto(task_ctx)?; | ||
| if source_proto.expr_id != target.expression_id() { | ||
| return internal_err!("dynamic filter update has a mismatched expression ID"); | ||
| } | ||
| let Some(ExprType::DynamicFilter(source_dynamic_filter)) = source_proto.expr_type else { | ||
| return internal_err!("expected a dynamic filter update"); | ||
| }; | ||
| let Some(source_predicate) = source_dynamic_filter.inner_expr.as_deref() else { | ||
| return internal_err!("dynamic filter update has no predicate"); | ||
| }; | ||
| let predicate = decode_physical_expr(source_predicate, source_schema, task_ctx)?; | ||
|
|
||
| let source_original_children = source_dynamic_filter | ||
| .children | ||
| .iter() | ||
| .map(|child| decode_physical_expr(child, source_schema, task_ctx)) | ||
| .collect::<Result<Vec<_>>>()?; | ||
|
|
||
| let update_target = Arc::clone(target).with_new_children(source_original_children)?; | ||
| let Ok(update_target) = Arc::downcast::<DynamicFilterPhysicalExpr>(update_target) else { | ||
| return internal_err!("expected a dynamic filter update target"); | ||
| }; | ||
| update_target.update(predicate)?; | ||
| if source_dynamic_filter.is_complete { | ||
| update_target.mark_complete(); | ||
| } | ||
| Ok(()) | ||
| } |
This file contains hidden or 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.
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.
For aggregates and sorts, this is tested via the integration test. Also TPCDS has a ton of queries that should exercise this:
q1, q7, q8, q15, q26, q27, q37, q43, q50, q58, q69, q74, q82, q83, q84