You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "Satyr09 (via GitHub)" <gi...@apache.org> on 2026/05/30 15:46:10 UTC
[PR] feat: add max_row_group_bytes option to ParquetOptions [datafusion]
Satyr09 opened a new pull request, #22649:
URL: https://github.com/apache/datafusion/pull/22649
## Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
-->
- Closes #.
## Rationale for this change
<!--
Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.
-->
arrow-rs 58.0 added WriterProperties::set_max_row_group_bytes (PR: apache/arrow-rs#9357
Issue: apache/arrow-rs#1213), which flushes a row group when either the row-count or the byte limit is reached, whichever comes first, matching parquet-mr's parquet.block.size. DataFusion already consumes atleast this version of arrow but does not yet expose this new byte-based setter through its config.
## What changes are included in this PR?
<!--
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
-->
- Add `max_row_group_bytes: Option<usize>` (default None) to ParquetOptions in `datafusion/common/src/config.rs`.
- Wire it through `ParquetOptions::into_writer_properties_builder` to `WriterPropertiesBuilder::set_max_row_group_bytes`, with a guard that rejects Some(0) as a configuration error (arrow-rs panics on a zero byte limit).
- Update the generated config docs (configs.md, regenerated via dev/update_config_docs.sh) and the format options table (format_options.md).
## Are these changes tested?
<!--
We typically require tests for all PRs in order to:
1. Prevent the code from being accidentally broken by subsequent changes
2. Serve as another way to document the expected behavior of the code
If tests are not included in your PR, please explain why (for example, are they covered by existing tests)?
-->
Yes. New unit tests in datafusion/common/src/file_options/parquet_writer.rs cover:
- the option defaults to None (no byte limit propagated to WriterProperties),
- a configured value propagates to WriterProperties,
- Some(0) is rejected with a configuration error.
The existing round-trip test (`table_parquet_opts_to_writer_props`) and the defaults test (`test_defaults_match`) were extended to include the new field, so serialization round-trips and default consistency are
exercised. Verified locally with:
`cargo test -p datafusion-common --features parquet`
## Are there any user-facing changes?
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
Additive only, does not affect existing options.
<!--
If there are any breaking changes to public APIs, please add the `api change` label.
-->
--
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] feat: add max_row_group_bytes option to ParquetOptions [datafusion]
Posted by "2010YOUY01 (via GitHub)" <gi...@apache.org>.
2010YOUY01 commented on code in PR #22649:
URL: https://github.com/apache/datafusion/pull/22649#discussion_r3329599018
##########
datafusion/common/src/file_options/parquet_writer.rs:
##########
@@ -213,6 +214,12 @@ impl ParquetOptions {
max_predicate_cache_size: _,
} = self;
+ if let Some(0) = max_row_group_bytes {
Review Comment:
I recommend to move all the config related validation to
- datafusion/common/src/config.rs
You can find examples in `config.rs` -- there are configurations that are custom type, and a `FromStr` trait is implemented on them.
##########
datafusion/sqllogictest/test_files/parquet_max_row_group_bytes.slt:
##########
@@ -0,0 +1,83 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+
+# http://www.apache.org/licenses/LICENSE-2.0
+
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+# End-to-end tests for the `max_row_group_bytes` Parquet writer option:
Review Comment:
It would be great to add the following coverages:
- When read back, check the row group count -- for example if `max_row_group_bytes` was set to `1`, there will be 5 row groups created
- Test the combination of `max_row_group_size` and `max_row_group_bytes` for the 'whichever reaches first' semantics.
##########
datafusion/common/src/config.rs:
##########
@@ -976,6 +976,15 @@ config_namespace! {
/// can get better compression and be faster to read.
pub max_row_group_size: usize, default = 1024 * 1024
+ /// (writing) Target maximum size of each row group in bytes. When set,
+ /// the writer flushes whenever either this limit or `max_row_group_size`
+ /// is reached, whichever comes first. Useful for bounding writer memory
Review Comment:
Let's also update `max_row_group_size` for this 'whichever comes first` semantics.
--
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] feat: add max_row_group_bytes option to ParquetOptions [datafusion]
Posted by "2010YOUY01 (via GitHub)" <gi...@apache.org>.
2010YOUY01 commented on PR #22649:
URL: https://github.com/apache/datafusion/pull/22649#issuecomment-4621791343
> I am working on a follow up that teaches the parallel writer in DF to honour this config as well - but I suspect that would be a larger change and might be better served by being its own PR.
I think this is a good idea, I suggest to:
1. update the doc for the limitation
2. Open an issue for the follow-up work
3. Merge this PR first, then iterate on that
--
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] feat: add max_row_group_bytes option to ParquetOptions [datafusion]
Posted by "Satyr09 (via GitHub)" <gi...@apache.org>.
Satyr09 commented on PR #22649:
URL: https://github.com/apache/datafusion/pull/22649#issuecomment-4621624724
Hi @2010YOUY01 , thank you so much for your reviews, and the approval!
Heads-up before merge: while adding the end-to-end tests, I found that `max_row_group_bytes` is currently honored only by the single-threaded Parquet writer.
The reason, as far as my understanding goes is that the serial path hands the whole write loop to arrow-rs's `ArrowWriter` (boundary logic and all), so the byte-flush is free. The parallel path reimplements the row-group boundary loop itself, which only ever consulted `max_row_group_row_count` - so `max_row_group_bytes` was never checked.
This PR's e2e tests reflect that - we do ``allow_single_file_parallelism = false` in order to run e2e testing honouring the new config.
I am working on a follow up that teaches the parallel writer in DF to honour this config as well - but I suspect that would be a larger change and might be better served by being its own PR.
Given this, would you still be okay with merging this as is (with a note mentioning that this config currently only works when allow_single_file_parallelism is also switched off)?
--
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] feat: add max_row_group_bytes option to ParquetOptions [datafusion]
Posted by "Satyr09 (via GitHub)" <gi...@apache.org>.
Satyr09 commented on PR #22649:
URL: https://github.com/apache/datafusion/pull/22649#issuecomment-4622520751
> > I am working on a follow up that teaches the parallel writer in DF to honour this config as well - but I suspect that would be a larger change and might be better served by being its own PR.
>
> I think this is a good idea, I suggest to:
>
> 1. update the doc for the limitation
> 2. Open an issue for the follow-up work
> 3. Merge this PR first, then iterate on that
Thanks @2010YOUY01 , added
"Currently only honored when `allow_single_file_parallelism` is `false`; by default the parallel file writer ignores this limit. "
to the documentation for the config.
Will create the follow-up issue/PR soon.
--
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] feat: add max_row_group_bytes option to ParquetOptions [datafusion]
Posted by "2010YOUY01 (via GitHub)" <gi...@apache.org>.
2010YOUY01 merged PR #22649:
URL: https://github.com/apache/datafusion/pull/22649
--
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] feat: add max_row_group_bytes option to ParquetOptions [datafusion]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #22649:
URL: https://github.com/apache/datafusion/pull/22649#issuecomment-4585496697
<!-- 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-common v53.1.0 (current)
Built [ 33.331s] (current)
Parsing datafusion-common v53.1.0 (current)
Parsed [ 0.063s] (current)
Building datafusion-common v53.1.0 (baseline)
Built [ 32.020s] (baseline)
Parsing datafusion-common v53.1.0 (baseline)
Parsed [ 0.061s] (baseline)
Checking datafusion-common v53.1.0 -> v53.1.0 (no change; assume patch)
Checked [ 0.887s] 222 checks: 221 pass, 1 fail, 0 warn, 30 skip
--- failure constructible_struct_adds_field: externally-constructible struct adds field ---
Description:
A pub struct constructible with a struct literal has a new pub field. Existing struct literals must be updated to include the new field.
ref: https://doc.rust-lang.org/reference/expressions/struct-expr.html
impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.47.0/src/lints/constructible_struct_adds_field.ron
Failed in:
field ParquetOptions.max_row_group_bytes in /home/runner/work/datafusion/datafusion/datafusion/common/src/config.rs:843
Summary semver requires new major version: 1 major and 0 minor checks failed
Finished [ 68.313s] datafusion-common
Building datafusion-proto v53.1.0 (current)
Built [ 56.417s] (current)
Parsing datafusion-proto v53.1.0 (current)
Parsed [ 0.020s] (current)
Building datafusion-proto v53.1.0 (baseline)
Built [ 56.198s] (baseline)
Parsing datafusion-proto v53.1.0 (baseline)
Parsed [ 0.020s] (baseline)
Checking datafusion-proto v53.1.0 -> v53.1.0 (no change; assume patch)
Checked [ 0.344s] 222 checks: 222 pass, 30 skip
Summary no semver update required
Finished [ 114.542s] datafusion-proto
Building datafusion-proto-common v53.1.0 (current)
Built [ 20.400s] (current)
Parsing datafusion-proto-common v53.1.0 (current)
Parsed [ 0.051s] (current)
Building datafusion-proto-common v53.1.0 (baseline)
Built [ 20.654s] (baseline)
Parsing datafusion-proto-common v53.1.0 (baseline)
Parsed [ 0.052s] (baseline)
Checking datafusion-proto-common v53.1.0 -> v53.1.0 (no change; assume patch)
Checked [ 1.379s] 222 checks: 221 pass, 1 fail, 0 warn, 30 skip
--- failure constructible_struct_adds_field: externally-constructible struct adds field ---
Description:
A pub struct constructible with a struct literal has a new pub field. Existing struct literals must be updated to include the new field.
ref: https://doc.rust-lang.org/reference/expressions/struct-expr.html
impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.47.0/src/lints/constructible_struct_adds_field.ron
Failed in:
field ParquetOptions.max_row_group_bytes_opt in /home/runner/work/datafusion/datafusion/datafusion/proto-common/src/generated/prost.rs:904
field ParquetOptions.max_row_group_bytes_opt in /home/runner/work/datafusion/datafusion/datafusion/proto-common/src/generated/prost.rs:904
field ParquetOptions.max_row_group_bytes_opt in /home/runner/work/datafusion/datafusion/datafusion/proto-common/src/generated/prost.rs:904
Summary semver requires new major version: 1 major and 0 minor checks failed
Finished [ 43.843s] datafusion-proto-common
Building datafusion-proto-models v53.1.0 (current)
Built [ 23.669s] (current)
Parsing datafusion-proto-models v53.1.0 (current)
Parsed [ 0.129s] (current)
Building datafusion-proto-models v53.1.0 (baseline)
Built [ 23.470s] (baseline)
Parsing datafusion-proto-models v53.1.0 (baseline)
Parsed [ 0.131s] (baseline)
Checking datafusion-proto-models v53.1.0 -> v53.1.0 (no change; assume patch)
Checked [ 2.218s] 222 checks: 221 pass, 1 fail, 0 warn, 30 skip
--- failure constructible_struct_adds_field: externally-constructible struct adds field ---
Description:
A pub struct constructible with a struct literal has a new pub field. Existing struct literals must be updated to include the new field.
ref: https://doc.rust-lang.org/reference/expressions/struct-expr.html
impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.47.0/src/lints/constructible_struct_adds_field.ron
Failed in:
field ParquetOptions.max_row_group_bytes_opt in /home/runner/work/datafusion/datafusion/datafusion/proto-models/src/generated/datafusion_proto_common.rs:904
field ParquetOptions.max_row_group_bytes_opt in /home/runner/work/datafusion/datafusion/datafusion/proto-models/src/generated/datafusion_proto_common.rs:904
Summary semver requires new major version: 1 major and 0 minor checks failed
Finished [ 50.950s] datafusion-proto-models
Building datafusion-sqllogictest v53.1.0 (current)
Built [ 168.628s] (current)
Parsing datafusion-sqllogictest v53.1.0 (current)
Parsed [ 0.024s] (current)
Building datafusion-sqllogictest v53.1.0 (baseline)
Built [ 168.478s] (baseline)
Parsing datafusion-sqllogictest v53.1.0 (baseline)
Parsed [ 0.026s] (baseline)
Checking datafusion-sqllogictest v53.1.0 -> v53.1.0 (no change; assume patch)
Checked [ 0.114s] 222 checks: 222 pass, 30 skip
Summary no semver update required
Finished [ 343.407s] datafusion-sqllogictest
```
</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] feat: add max_row_group_bytes option to ParquetOptions [datafusion]
Posted by "2010YOUY01 (via GitHub)" <gi...@apache.org>.
2010YOUY01 commented on code in PR #22649:
URL: https://github.com/apache/datafusion/pull/22649#discussion_r3329599018
##########
datafusion/common/src/file_options/parquet_writer.rs:
##########
@@ -213,6 +214,12 @@ impl ParquetOptions {
max_predicate_cache_size: _,
} = self;
+ if let Some(0) = max_row_group_bytes {
Review Comment:
I recommend to move all the config related validation to
- datafusion/common/src/config.rs
You can find examples in `config.rs` -- there are configurations that are custom type, and a `FromStr` trait is implemented on them.
--
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