Proto: migrate MemorySourceConfig to per-source try_to_proto / try_from_proto hooks - #24187
Merged
adriangb merged 4 commits intoAug 9, 2026
Merged
Conversation
Implement DataSource::try_to_proto for MemorySourceConfig and the inherent MemorySourceConfig::try_from_proto, moving the MemoryScan wire logic into the crate that owns the type. The record-batch IPC serde is pure Arrow and is inlined locally. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Decode dispatches to MemorySourceConfig::try_from_proto; the central MemoryScan encode arm in try_from_data_source_exec is deleted so the DataSource::try_to_proto hook is the only path. The old decode helper becomes a deprecated shim delegating to the new hook, and the now-unused record-batch IPC helpers are deprecated. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The display-string roundtrip does not cover every field, so the decoded source is downcast and its fetch, show_sizes and sort_information are asserted directly. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
github-actions
Bot
added
proto
|
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 |
Contributor
Author
|
@kumarUjjawal could I bother you for a review please 🙏🏻 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #24187 +/- ##
==========================================
- Coverage 81.05% 81.04% -0.02%
==========================================
Files 1107 1106 -1
Lines 381574 382315 +741
Branches 381574 382315 +741
==========================================
+ Hits 309281 309842 +561
- Misses 54034 54159 +125
- Partials 18259 18314 +55 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
kumarUjjawal
approved these changes
Aug 8, 2026
kumarUjjawal
left a comment
kumarUjjawal
left a comment
Contributor
There was a problem hiding this comment.
Thanks @adriangb
Looks good, I left one non-blocking comments.
| .expect("expected MemorySourceConfig"); | ||
| assert_eq!(decoded_source.fetch(), Some(1)); | ||
| assert!(!decoded_source.show_sizes()); | ||
| assert_eq!(decoded_source.sort_information().len(), 1); |
Contributor
There was a problem hiding this comment.
Could this compare the decoded partitions, schema/projection, and complete ordering rather than only the ordering count?
Review feedback: assert equality of partitions, schema, projection and the complete sort information against the original source, not just counts. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Which issue does this PR close?
Rationale for this change
Continues the per-plan proto hook migration (#23494, pattern established in #23495 and #23683): each plan/source owns its wire logic via
try_to_proto/try_from_protohooks instead of the central downcast/match chains indatafusion-proto.What changes are included in this PR?
DataSource::try_to_protoimplemented forMemorySourceConfigand inherentMemorySourceConfig::try_from_protoadded indatafusion/datasource/src/memory.rs(behind theprotofeature). The record-batch IPC serde is pure Arrow and is inlined locally.MemoryScandecode arm indatafusion-protonow delegates toMemorySourceConfig::try_from_proto, and the centralMemoryScanencode arm intry_from_data_source_execis deleted — proof that the hook is the only path.try_into_memory_scan_physical_planbecomes a deprecated shim delegating to the new hook; the now-unusedserialize_record_batches/parse_record_batcheshelpers are deprecated.The wire format is byte-for-byte identical: I verified locally that a memory scan plan (with sort information, fetch, and
show_sizes=false) encodes to identical bytes before and after this change, and that bytes encoded by each version decode correctly with the other.Are these changes tested?
Yes. Existing roundtrip tests (
roundtrip_memory_source,roundtrip_memory_source_empty_projection) now exercise the hooks since the central arms are gone. A new testroundtrip_memory_source_sort_information_and_fetchcoverssort_information,fetch, andshow_sizes, asserting on the decodedMemorySourceConfigdirectly rather than only the display string.Are there any user-facing changes?
No behavior changes.
PhysicalPlanNodeExt::try_into_memory_scan_physical_plan,parse_record_batches, andserialize_record_batchesare deprecated (still functional).🤖 Generated with Claude Code