You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "buraksenn (via GitHub)" <gi...@apache.org> on 2026/08/31 06:11:43 UTC
[PR] refactor(proto): destructure FileScanConfig and MemorySourceConfig serde hooks [datafusion]
buraksenn opened a new pull request, #24813:
URL: https://github.com/apache/datafusion/pull/24813
## Which issue does this PR close?
- Closes #24624.
## Rationale for this change
Proto hooks that access fields individually can silently omit newly added state. Exhaustive destructuring makes such omissions compile errors.
`FileScanConfig` is shared by all file sources, so a field omitted here is lost for Parquet, CSV, JSON, Arrow, and Avro.
## What changes are included in this PR?
- Exhaustively destructure `FileScanConfig` and `MemorySourceConfig` in their encoders.
- Exhaustively destructure their protobuf nodes in the decoders.
- Document fields that are serialized indirectly or reconstructed during decoding.
- Add an optional `preserve_order` field so explicit values survive file scan round trips while older payloads retain their ordering-derived behavior.
- Reject default serialization of non-default expression adapter factories instead of silently discarding their behavior, and update the custom serialization example accordingly.
- Decode projected `MemorySourceConfig` sort information against the projected schema without applying projection twice.
The protobuf change is additive and backward compatible.
## Are these changes tested?
Added or updated coverage for:
- explicit `preserve_order` values and legacy payload behavior
- default and custom expression adapter factories
- projected memory-source sort information, projection, fetch, and display settings
## Are there any user-facing changes?
File scans now preserve an explicit `preserve_order` value across protobuf round trips. Default serialization of plans containing custom expression adapter factories now returns an explicit error rather than silently dropping the factory.
Projected memory sources now preserve their sort information across round trips. The new protobuf field is additive, and `PhysicalExprAdapterFactory` gains a default method, so existing implementations remain source compatible.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] refactor(proto): destructure FileScanConfig and MemorySourceConfig serde hooks [datafusion]
Posted by "kosiew (via GitHub)" <gi...@apache.org>.
kosiew merged PR #24813:
URL: https://github.com/apache/datafusion/pull/24813
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] refactor(proto): destructure FileScanConfig and MemorySourceConfig serde hooks [datafusion]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #24813:
URL: https://github.com/apache/datafusion/pull/24813#issuecomment-5474669388
## [Codecov](https://app.codecov.io/gh/apache/datafusion/pull/24813?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `82.11382%` with `44 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 81.52%. Comparing base ([`9ef3d47`](https://app.codecov.io/gh/apache/datafusion/commit/9ef3d47225b6b3426a0a4e34518115a984a3ecc9?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`b7c55a0`](https://app.codecov.io/gh/apache/datafusion/commit/b7c55a0249345a96519563f0d5054b7820ae5032?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 10 commits behind head on main.
| [Files with missing lines](https://app.codecov.io/gh/apache/datafusion/pull/24813?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [datafusion/proto/src/physical\_plan/mod.rs](https://app.codecov.io/gh/apache/datafusion/pull/24813?src=pr&el=tree&filepath=datafusion%2Fproto%2Fsrc%2Fphysical_plan%2Fmod.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9wcm90by9zcmMvcGh5c2ljYWxfcGxhbi9tb2QucnM=) | 64.58% | [10 Missing and 7 partials :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/24813?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [datafusion/proto-models/src/generated/pbjson.rs](https://app.codecov.io/gh/apache/datafusion/pull/24813?src=pr&el=tree&filepath=datafusion%2Fproto-models%2Fsrc%2Fgenerated%2Fpbjson.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9wcm90by1tb2RlbHMvc3JjL2dlbmVyYXRlZC9wYmpzb24ucnM=) | 0.00% | [13 Missing :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/24813?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [datafusion/datasource/src/memory.rs](https://app.codecov.io/gh/apache/datafusion/pull/24813?src=pr&el=tree&filepath=datafusion%2Fdatasource%2Fsrc%2Fmemory.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9kYXRhc291cmNlL3NyYy9tZW1vcnkucnM=) | 87.50% | [1 Missing and 7 partials :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/24813?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...atafusion/datasource/src/file\_scan\_config/proto.rs](https://app.codecov.io/gh/apache/datafusion/pull/24813?src=pr&el=tree&filepath=datafusion%2Fdatasource%2Fsrc%2Ffile_scan_config%2Fproto.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9kYXRhc291cmNlL3NyYy9maWxlX3NjYW5fY29uZmlnL3Byb3RvLnJz) | 94.78% | [0 Missing and 6 partials :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/24813?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #24813 +/- ##
==========================================
- Coverage 81.52% 81.52% -0.01%
==========================================
Files 1123 1123
Lines 405970 406349 +379
Branches 405970 406349 +379
==========================================
+ Hits 330983 331277 +294
- Misses 55626 55689 +63
- Partials 19361 19383 +22
```
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/datafusion/pull/24813?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] refactor(proto): destructure FileScanConfig and MemorySourceConfig serde hooks [datafusion]
Posted by "kosiew (via GitHub)" <gi...@apache.org>.
kosiew commented on PR #24813:
URL: https://github.com/apache/datafusion/pull/24813#issuecomment-5581656848
🚀
@buraksenn
Thank you for your contribution.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] refactor(proto): destructure FileScanConfig and MemorySourceConfig serde hooks [datafusion]
Posted by "buraksenn (via GitHub)" <gi...@apache.org>.
buraksenn commented on code in PR #24813:
URL: https://github.com/apache/datafusion/pull/24813#discussion_r3951187424
##########
datafusion/proto/src/physical_plan/mod.rs:
##########
@@ -374,6 +400,55 @@ mod file_scan_config_serde {
Ok(())
}
+ #[test]
+ fn new_file_scan_config_serde_preserves_explicit_order_flag() -> Result<()> {
Review Comment:
Thanks for the review. I've added additional test case for this
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] refactor(proto): destructure FileScanConfig and MemorySourceConfig serde hooks [datafusion]
Posted by "adriangb (via GitHub)" <gi...@apache.org>.
adriangb commented on PR #24813:
URL: https://github.com/apache/datafusion/pull/24813#issuecomment-5573060855
Thanks @buraksenn and @kosiew !
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] refactor(proto): destructure FileScanConfig and MemorySourceConfig serde hooks [datafusion]
Posted by "kosiew (via GitHub)" <gi...@apache.org>.
kosiew commented on code in PR #24813:
URL: https://github.com/apache/datafusion/pull/24813#discussion_r3947578226
##########
datafusion/proto/src/physical_plan/mod.rs:
##########
@@ -374,6 +400,55 @@ mod file_scan_config_serde {
Ok(())
}
+ #[test]
+ fn new_file_scan_config_serde_preserves_explicit_order_flag() -> Result<()> {
Review Comment:
Optional: could we add protobuf JSON coverage for `preserve_order: Some(false)` and the legacy case where the field is absent? The `json` feature supports this representation, and the generated `pbjson` code preserves the distinction between an explicit `false` and an absent value.
I think this would be useful for protecting the generated JSON/codegen contract. A test in `datafusion-proto-models` compiled with its `json` feature would probably be the best place for it, rather than testing the handwritten serde path here. Not required for this PR.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] refactor(proto): destructure FileScanConfig and MemorySourceConfig serde hooks [datafusion]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #24813:
URL: https://github.com/apache/datafusion/pull/24813#issuecomment-5474518950
<!-- semver-check-comment -->
Thank you for opening this pull request!
Reviewer note: [cargo-semver-checks](https://github.com/obi1kenobi/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>
<summary>Details</summary>
```
Cloning apache/main
Building datafusion-datasource v55.0.0 (current)
Built [ 42.709s] (current)
Parsing datafusion-datasource v55.0.0 (current)
Parsed [ 0.030s] (current)
Building datafusion-datasource v55.0.0 (baseline)
Built [ 42.743s] (baseline)
Parsing datafusion-datasource v55.0.0 (baseline)
Parsed [ 0.030s] (baseline)
Checking datafusion-datasource v55.0.0 -> v55.0.0 (no change; assume patch)
Checked [ 0.313s] 223 checks: 223 pass, 31 skip
Summary no semver update required
Finished [ 87.531s] datafusion-datasource
Building datafusion-physical-expr-adapter v55.0.0 (current)
Built [ 32.062s] (current)
Parsing datafusion-physical-expr-adapter v55.0.0 (current)
Parsed [ 0.010s] (current)
Building datafusion-physical-expr-adapter v55.0.0 (baseline)
Built [ 32.030s] (baseline)
Parsing datafusion-physical-expr-adapter v55.0.0 (baseline)
Parsed [ 0.010s] (baseline)
Checking datafusion-physical-expr-adapter v55.0.0 -> v55.0.0 (no change; assume patch)
Checked [ 0.102s] 223 checks: 223 pass, 31 skip
Summary no semver update required
Finished [ 65.338s] datafusion-physical-expr-adapter
Building datafusion-proto v55.0.0 (current)
Built [ 53.030s] (current)
Parsing datafusion-proto v55.0.0 (current)
Parsed [ 0.017s] (current)
Building datafusion-proto v55.0.0 (baseline)
Built [ 53.245s] (baseline)
Parsing datafusion-proto v55.0.0 (baseline)
Parsed [ 0.018s] (baseline)
Checking datafusion-proto v55.0.0 -> v55.0.0 (no change; assume patch)
Checked [ 0.143s] 223 checks: 223 pass, 31 skip
Summary no semver update required
Finished [ 107.858s] datafusion-proto
Building datafusion-proto-models v55.0.0 (current)
Built [ 24.837s] (current)
Parsing datafusion-proto-models v55.0.0 (current)
Parsed [ 0.126s] (current)
Building datafusion-proto-models v55.0.0 (baseline)
Built [ 24.755s] (baseline)
Parsing datafusion-proto-models v55.0.0 (baseline)
Parsed [ 0.126s] (baseline)
Checking datafusion-proto-models v55.0.0 -> v55.0.0 (no change; assume patch)
Checked [ 2.203s] 223 checks: 222 pass, 1 fail, 0 warn, 31 skip
--- failure constructible_struct_adds_field: struct exhaustively constructible through public API adds field ---
Description:
A pub struct that could be exhaustively constructed with a literal using only public API has a new pub field, breaking existing exhaustive literals.
ref: https://doc.rust-lang.org/reference/expressions/struct-expr.html
impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.50.0/src/lints/constructible_struct_adds_field.ron
Failed in:
field FileScanExecConf.preserve_order in /home/runner/work/datafusion/datafusion/datafusion/proto-models/src/generated/prost.rs:1965
field FileScanExecConf.preserve_order in /home/runner/work/datafusion/datafusion/datafusion/proto-models/src/generated/prost.rs:1965
Summary semver requires new major version: 1 major and 0 minor checks failed
Finished [ 53.321s] datafusion-proto-models
```
</details>
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org