fix: flush FilterExec output on pending inut for unbounded inputs - #24353
fix: flush FilterExec output on pending inut for unbounded inputs#24353goutamadwant wants to merge 3 commits into
Conversation
Was that only to buffer up to |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #24353 +/- ##
==========================================
- Coverage 81.61% 81.61% -0.01%
==========================================
Files 1124 1124
Lines 411978 412179 +201
Branches 411978 412179 +201
==========================================
+ Hits 336236 336384 +148
- Misses 55936 55957 +21
- Partials 19806 19838 +32 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
de069c7 to
3289f3f
Compare
|
It seems like belonged to the issue under #24265 |
|
I think this behavior is a design change, and now most operators don't follow. I don't understand why this is the expected behavior, the existing behavior seems more reasonable, as micro-batching helps vectorize with small latency penalties. I suggest to have some further discussion before proceeding. |
alamb
left a comment
There was a problem hiding this comment.
Thank you @goutamadwant -- i think this makes sense, but we should get alignment on what the intention of Unbounded input is and make sure @2010YOUY01 agrees / understands the rationale before we merge this
| context.task_id() | ||
| ); | ||
| let metrics = FilterExecMetrics::new(&self.metrics, partition); | ||
| let flush_when_input_pending = self.input.boundedness().is_unbounded(); |
There was a problem hiding this comment.
I think this is the key change / question --
I think the assumption is that if the input is "unbounded" that signals we are in " streaming mode" and thus the users are prioritizing latency over raw throughput.
However, looking at the Boundedness documentation that is never explicitly stated -- perhaps we should update it (maybe @jayzhan211 who introduced it in #13823 could confirm the intent)
datafusion/datafusion/physical-plan/src/execution_plan.rs
Lines 1289 to 1314 in caa3cb4
| /// Batch coalescer to combine small batches | ||
| batch_coalescer: LimitedBatchCoalescer, | ||
| /// Emit buffered rows when an unbounded input has no batch ready. | ||
| flush_when_input_pending: bool, |
There was a problem hiding this comment.
I recommend renaming this to something like input_unbounded to reflect what it represents rather than the behavior that it controls
| None => { | ||
| match self.input.poll_next_unpin(cx) { | ||
| Poll::Pending => { | ||
| if self.flush_when_input_pending && !self.batch_coalescer.is_empty() { |
There was a problem hiding this comment.
maybe we could add a comment here explaining the rationale -- something like
// When the input is unbounded / streaming, flush any internal buffered
// batches so the rest of the pipeline can produce output if possible.| use std::time::Duration; | ||
| use tokio::sync::mpsc::{Receiver, Sender, channel}; | ||
|
|
||
| struct ChannelPartition { |
There was a problem hiding this comment.
I wonder if we can use a shared fixture somewhere for this -- it seems pretty general rather than specific to filter
Yes, I want to know more about application level motivation, and why the existing behavior won't satisfy the requirement, and next see what should we do. Another concern is if we start to incorporate such no buffering behavior, extra implementation complexity will be added to all existing operators, for this Is it possible to split it to a |
That's exactly what I've done, but @alamb's point is that more projects use DataFusion in the streaming context, so it would be nice to support this out of the box. |
#24044 shows the issue that arises if we don't support "streaming". If we agree on supporting more "streaming" execution in DataFusion, I think we should extend that support across all existing operators. +1 from me on the overall direction. How to design this while keeping maintenance complexity low could be a separate discussion — maybe in #24265 |
|
@jayzhan211 thanks for the pointer to that EPIC My main blocking suggestion for this PR is to completely separate the bounded and unbounded execution paths, rather than adding the unbounded behavior into the existing implementation. If we can do that, I think this PR should be good to go. I explained the reasoning in more detail in the comment in #24265 (comment) |
Yeah -- in my mind DataFusion is in a half way state now -- it has some features / support for streaming (e.g. So I think we should first agree on if we want to try and make DataFusion more useful for building streaming engines -- I think it is important and there are a bunch of people already doing so (and have done so for a while -- e.g. Arroyo and earlier versions of Synnada). Rewriting Apache Flink in rust seems to be all the rage now too (e.g. @jordepic 's StreamFusion, etc) , and many people are using DataFusion to try.
|
|
I propose putting this stuff into a new crate ( |
|
ba07df5 overall LGTM |
|
Was that ready for review? Seems still WIP from the latest change 🤔 |
Which issue does this PR close?
UNBOUNDEDtables) #9016.Rationale for this change
While prototyping the minimal push-based streaming example proposed in #9016, I found that
FilterExeccould buffer small filtered results indefinitely when its input was unbounded. Because an unbounded source may never finish, consumers could not observe those results while continuing to push input batches.This PR extracts the smallest prerequisite fix so the streaming example and user-guide documentation can follow as separate, focused PRs.
This is complementary to #23856: that PR changes how filtered batches are supplied to the coalescer, while this PR controls when buffered rows become observable for an unbounded input.
What changes are included in this PR?
LimitedBatchCoalescerflush operation that emits buffered rows while still allowing subsequent input.FilterExecoutput after each input batch when its input is unbounded.Planned follow-up PRs:
datafusion-examples, showing how to feed batches into a running query and consume results incrementally.Are these changes tested?
Yes.
cargo test -p datafusion-physical-plancargo test -p datafusioncargo test -p datafusion-clicargo test --profile=ci --test sqllogictestsavro,json,backtrace,extended_tests,recursive_protection,parquet_encryptioncargo clippy --all-targets --all-features -- -D warningscargo fmt --all -- --checkAre there any user-facing changes?
Yes.
FilterExecnow emits filtered results incrementally for unbounded inputs instead of waiting for the target batch size or input completion.Bounded-input coalescing remains unchanged. There are no public API or breaking changes.