andygrove opened a new pull request, #23643:
URL: https://github.com/apache/datafusion/pull/23643
## Which issue does this PR close?
- Closes #23642.
## Rationale for this change
`EmptyExecNode` and `PlaceholderRowExecNode` encoded only a schema, so the
partition count set by `EmptyExec::with_partitions(n)` was silently lost across
a physical-plan round-trip: a plan reporting `n` partitions before
serialization reported `1` after.
This is silent data loss rather than an error — the decoded plan is
well-formed but describes a different plan than the one encoded. It bites any
setup that plans in one process and executes in another. In Ballista, an
optimizer rule collapses provably-empty sub-plans into an `EmptyExec`
inheriting the replaced node's partition count; the scheduler sizes the stage's
task count from that count, ships the plan to an executor, and every task above
partition 0 fails with:
```
Internal("Assertion failed: partition < self.partitions: EmptyExec invalid
partition 1 (expected less than 1)")
```
## What changes are included in this PR?
- Add a `uint32 partitions` field to `EmptyExecNode` and
`PlaceholderRowExecNode` in `datafusion.proto`, and regenerate the prost/pbjson
code via `regen.sh`.
- Add a public `partitions()` accessor to `EmptyExec` and
`PlaceholderRowExec`, since the field was private and the encoder had no way to
read it.
- Write `partitions` on encode and apply it via `with_partitions(...)` on
decode.
The wire format stays compatible in both directions. Plans encoded before
this field existed carry no value for it, which prost surfaces as `0`; decode
maps that to the previous default of `1`. Plans encoded after this change add a
field that older readers skip.
Note for #23501, which migrates these nodes to the `try_to_proto` /
`try_from_proto` pattern: that issue specifies "schema only" and a
byte-for-byte identical wire format, which would reintroduce this bug. The
`partitions` field should be folded into that rewrite.
## Are these changes tested?
Yes, three new tests in `roundtrip_physical_plan.rs`: partition-count
round-trips for `EmptyExec` and `PlaceholderRowExec`, plus one decoding
`partitions: 0` nodes directly to pin the backward-compatibility mapping. The
round-trip tests were confirmed to fail against the unfixed decoder with
exactly the reported symptom (4 partitions decoding to 1).
## Are there any user-facing changes?
`EmptyExec::partitions()` and `PlaceholderRowExec::partitions()` are new
public accessors; no existing API changes. Encoded plans gain a new protobuf
field, which is backward and forward compatible as described above.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
