Repository navigation
Conversation
github-actions
Bot
added
development-process
|
Thank you for opening this pull request! Reviewer note: cargo-semver-checks reported the current version number is not SemVer-compatible with the changes in this pull request (compared against the base branch). Details |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #25765 +/- ##
==========================================
+ Coverage 82.84% 82.91% +0.06%
==========================================
Files 1151 1155 +4
Lines 455961 457653 +1692
Branches 455961 457653 +1692
==========================================
+ Hits 377760 379460 +1700
+ Misses 55245 55186 -59
- Partials 22956 23007 +51 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Thanks for this refactor. Moving generate-series serialization into the generator hooks simplifies the central protobuf code while preserving the existing wire format, execution behavior, and extension fallback.
I found no blocking correctness issues, so I'm approving with two optional suggestions. One compatibility concern worth highlighting is that removing four methods from PhysicalPlanNodeExt breaks downstream Rust callers. I'd prefer deprecated forwarding methods Please document the migration path.
| /// Returns a new instance with the state reset. | ||
| fn reset_state(&self) -> Arc<RwLock<dyn LazyBatchGenerator>>; | ||
|
|
||
| /// Encode this generator using the built-in generate-series wire format. |
There was a problem hiding this comment.
Could we clarify the contract for this hook? Returning Some(node) means the plan will be reconstructed as a built-in series generator, so its rows, batch boundaries, and schema should match the original generator after reset. It would also help to mention that Err stops serialization, while only Ok(None) allows extension fallback.
| } | ||
|
|
||
| #[test] | ||
| fn lazy_memory_dispatches_to_generator_hook() -> Result<()> { |
There was a problem hiding this comment.
Could we optionally add a custom generator that delegates execution and reset to a real series, then compare its rows, batch sizes, and schema before and after serialization? The current HookGenerator returns no rows even though its payload represents [3, 2, 1], so this would help demonstrate the execution contract for custom generators.
3 participants
Which issue does this PR close?
Part of #25395. This is the wire-compatible generator-hook step; multiple-generator serialization remains separate work.
Rationale for this change
Serializing a built-in
LazyMemoryExeccurrently requires the central protobuf implementation to recognize each concrete generator type. Let generators supply the existing generate-series representation, with decoding owned by the series implementation.What changes are included in this PR?
LazyBatchGenerator::try_to_protohook and dispatch to it fromLazyMemoryExec.datafusion-functions-table.PhysicalPlanNodeExtgenerate-series helpers as deprecated compatibility methods. The proto README documents replacements.Ok(None)permits extension fallback;Errstops serialization.The fixed built-in decoder still requires
datafusion-functions-table; removing that dependency would require a separate decoder registration design.What is the testing strategy for this PR?
Tests build legacy wire messages independently of the hooks and check exact re-encoding, execution, batching and repeatability. They also cover custom hook dispatch, deprecated helper compatibility, unsupported generators, malformed payloads and batch-size overflow.
Local validation of the review follow-up on Windows x64:
cargo test --locked -p datafusion-proto --test proto_integration lazy_memory: 6 passed, 0 failed.cargo fmt --all -- --check: passed.git diff --check: passed.cargo clippy --offline --all-targets --all-features -- -D warningsfailed with threeclippy::mutable_key_typeerrors in the untoucheddatafusion/optimizer/src/push_down_filter.rsat lines 1177, 1181 and 1665. The diagnostics followExprthrough Arrow buffers to memory-accounting interior mutability. That file has no changes in this follow-up; fetching current upstream main did not reveal a fix for these locations. Remaining build jobs were stopped after the errors. Full-workspace Clippy is not claimed as passing.The extended workspace test suite and performance benchmarks were not rerun. This follow-up adds compatibility forwarding methods, documentation and regression coverage; it does not change the aggregation/execution hot path.
Are there any user-facing changes?
New defaulted Rust trait methods and an opt-in
protofeature. Existing downstream generators need no changes. Direct users of the four old generate-series helpers retain source compatibility, with deprecation notices and a documented migration path. The built-in single-generator wire format is unchanged.This contribution is AI-assisted.