fix!(io): bound S3 write request size with a configurable part size by YuangGao · Pull Request #3179 · apache/iceberg-rust

YuangGao · 2026-09-09T06:38:02Z

Which issue does this PR close?

Part of opendal FileIO hard-wires TimeoutLayer defaults (10s per-IO) with no way to configure them; large Parquet row groups fail deterministically on S3 #2977.

What changes are included in this PR?

OpenDAL turns one caller buffer into one request, and ParquetWriter hands over a whole row group per write call. A 128 MiB row group therefore became a single UploadPart racing the hard-coded 10s IO timeout, and since every retry re-sends the same oversized request, the write fails deterministically rather than flakily.

Bound the request size so it follows configuration instead of the caller's buffer:

Add s3.multipart.part-size-bytes, default 32 MiB (matching Java S3FileIOProperties.MULTIPART_SIZE_DEFAULT), rejected below the 5 MiB S3 minimum for a non-final part.
OpenDalStorage::S3 applies it through write_options / writer_options. Other backends keep OpenDAL's defaults.

Are these changes tested?

Unit tests for property parsing: default, override, 5 MiB boundary, non-numeric input.
Integration test against MinIO asserting one FileWrite::write is split into the configured number of upload parts, counted from the multipart ETag suffix.
Verified with mc admin trace: a 160 MiB Parquet write previously issued one request, now issues 5 x 32 MiB parts plus a remainder.

AI Disclosure

AI-assisted implementation.

anoopj

CI check-public-api is failing. This PR changes a couple of public API surfaces that aren't reflected in the checked-in public-api.txt files.

Please run make generate-public-api and commit both updated files.

anoopj · 2026-09-09T15:56:58Z

        config: Arc<S3Config>,
+        /// Bytes carried by one multipart upload request.
+        #[serde(default = "default_multipart_part_size")]
+        multipart_part_size: usize,


This field is serialized and is round-tripped. A usize-typed serialized value ties the wire format to host word size. This might cause issues if we have heterogeneous setup (granted, they are rare). Consider storing u64 and converting as usize somewhere in the edge.

Good catch, fixed! Switched to u64.

…shots

YuangGao · 2026-09-16T01:50:44Z

@anoopj could you re-review this updated pr when you have a chance, thanks!

anoopj

LGTM now. Tagging committers @laskoviymishka @CTTY

CTTY

Thanks for the fix and the detailed info. I think we could thread the config in an easier way tho

CTTY · 2026-09-16T21:35:46Z

                customized_credential_load,
            } => Ok(Arc::new(OpenDalStorage::S3 {
                config: s3_config_parse(config.props().clone())?.into(),
+                multipart_part_size: s3_multipart_part_size_parse(config.props())?,


I think this is wrong. we don't need an extra field in the Storage, the config should be threaded via S3Config directly

S3Config here is opendal's own #[non_exhaustive] struct, so we can't add fields to it — and opendal doesn't model chunk size as service config anyway, it's a per-write option on WriteOptions. Since S3Config gets consumed by into_builder() when the Operator is built, the value has to be carried separately until the actual write call

let me know if you had something else in mind

Thanks for pointing this out! I think my concern still stands, we will always need to make breaking changes when adding a new config in the future. How about make the OpenDalStorage::S3 #[non_exhaustive]?

laskoviymishka

Thanks for this — the root-cause writeup is the best part, and I'm convinced by it: a row group handed to OpenDAL as one buffer becomes one UploadPart, and against the 10s per-op timeout a large enough row group fails deterministically. Bounding it with a configurable part size is the right shape, the Java parity on s3.multipart.part-size-bytes / 32 MiB default / 5 MiB floor is exact, and the regenerated public-api.txt reads cleanly. anoopj's already given it a pass, so I'm coming at this from the committer angle.

I'd like to settle a couple of things before we merge, though.

The one I care most about is the one-shot write() path. It now passes the same chunk option as the streaming writer, but as far as I can tell OpenDAL's write_options() is a single bulk write and doesn't split on chunk — only writer_options() does. If that's right, then we've fixed the real bug (the streaming ParquetWriter path, which the test covers) but the change to write() is a no-op that reads as if it does something, and I'd just drop it. If I'm wrong and it does chunk, then every small metadata write (manifests, snapshot JSON, table metadata) just picked up the 3x multipart round-trip, which I wouldn't want on commit-heavy workloads. Either way I'd like to know which it is and have a test pin it.

A few smaller things I'd want before merge:

an upper bound to match the 5 MiB floor — S3 caps a part at 5 GiB, and adding the ceiling also makes the 32-bit usize::try_from cast infallible
a call on whether multipart_part_size needs to be a public field or can be pub(crate) (users set it via the property either way)
the doc/units and glob-import nits inline

None of these are big; the core is solid. Fix the write()-path question and the upper bound and I'm happy to take another pass and approve.

laskoviymishka · 2026-09-17T07:23:53Z

    async fn write(&self, path: &str, bs: Bytes) -> Result<()> {
        let (op, relative_path) = self.create_operator(&path)?;
-        op.write(relative_path, bs)
+        op.write_options(relative_path, bs, self.write_options())


I think there's a subtle issue here: write_options() sets chunk, but per the OpenDAL docs chunk only takes effect on the streaming writer_options path — write_options() is a single bulk write and won't split a complete Bytes buffer into UploadPart calls no matter what chunk says.

If that's right, this line is misleading: the metadata writes that go through write() (manifests, snapshot JSON, table metadata) silently ignore the option, so we've only actually fixed the streaming writer() path. If it's not right and OpenDAL does chunk here, then every small metadata write now takes the 3x multipart round-trip (Create/Upload/Complete), which is a real regression on commit-heavy workloads.

Could we confirm which it is, and then either drop write_options() from write() if it's a no-op, or add a test through OutputFile::write() with a small part size asserting the part count? The existing test only covers writer(). wdyt?

Checked the source: write_options() and writer_options() build the same WriteContext and Writer — the one-shot version just does write + close, and the chunking happens in Writer::write, so write() does split. The regression doesn't happen either: MultipartWriter initiates lazily, so anything below the part size ends in write_once, a single PutObject.

also added test_file_io_s3_write_splits_buffer_into_bounded_parts and test_file_io_s3_write_below_part_size_stays_single_request against MinIO to pin both

laskoviymishka · 2026-09-17T07:23:53Z

+            format!("Invalid {S3_MULTIPART_PART_SIZE_BYTES}: {value}: {e}"),
+        )
+    })?;
+    if part_size < MULTIPART_PART_SIZE_MIN {


We validate the floor here but not the ceiling — S3 caps a part at 5 GiB, so something like 6_000_000_000 passes this check and then fails with an opaque S3 InvalidArgument mid-upload instead of a clean DataInvalid.

Adding a mirror check closes that, and it also makes the cast in write_options (usize::try_from(*multipart_part_size).unwrap_or(usize::MAX), lib.rs:415) infallible — right now on a 32-bit target a value above ~4 GiB silently clamps up to usize::MAX rather than erroring.

const MULTIPART_PART_SIZE_MAX: u64 = 5 * 1024 * 1024 * 1024;

Then the try_from can become .expect("validated <= 5 GiB"). Fix that and this is good to land.

Added MULTIPART_PART_SIZE_MAX and the mirror check. Kept unwrap_or(usize::MAX) rather than expect though — on a 32-bit target usize::MAX is under 4 GiB, so a validated 5 GiB value still fails the cast and the expect would be a panic reachable from config.

One correction: OpenDAL already clamps chunk to the service's write_multi_max_size (5 GiB for S3), so an oversized value was silently reduced rather than failing mid-upload

On the java side, I think anything larger than 2GB will fail java's parseInt, the javadoc also mentions : To ensure performance of the reader and writer, the part size must be less than 2GB..

I'm ok if we don't validate ceiling for now since opendal does it already. but this information may be good to go in to the comment section where we add the new multipart configs

laskoviymishka · 2026-09-17T07:23:53Z

        config: Arc<S3Config>,
+        /// Bytes carried by one multipart upload request.
+        #[serde(default = "default_multipart_part_size")]
+        multipart_part_size: u64,


Adding multipart_part_size as a required public field to the S3 variant is a source-breaking change — any external crate constructing OpenDalStorage::S3 { config, customized_credential_load } with an explicit field list stops compiling.

The crate is 0.x and you've already regenerated public-api.txt, so this is permitted, and customized_credential_load set a similar precedent. But since users configure this through the s3.multipart.part-size-bytes property (same as everything in config), I'd lean toward making the field pub(crate) and dropping it from public-api.txt — that keeps the constructor surface stable and matches how the rest of the config flows in.

If we'd rather keep it public, #[non_exhaustive] on the enum or a changelog note calling out the break would at least make it intentional. wdyt?

i think pub(crate) isn't possible here — enum variants and their fields always share the enum's visibility (E0449). Hiding it would mean a wrapper struct or #[non_exhaustive], but those break source compatibility too, so there's nothing to gain. I've left the field public and marked the commit fix! so the break shows up in the release notes

laskoviymishka · 2026-09-17T07:23:53Z

 pub const S3_DISABLE_CONFIG_LOAD: &str = "s3.disable-config-load";
+/// Size in bytes of each part of a multipart upload. Must be at least 5 MiB.
+/// Defaults to 32 MiB, matching Java `S3FileIOProperties.MULTIPART_SIZE`.
+pub const S3_MULTIPART_PART_SIZE_BYTES: &str = "s3.multipart.part-size-bytes";


While we're here — the TimeoutLayer default is 10s per IO op, and that applies per UploadPart, so part size is really the knob for how much has to transfer inside that window. Since that interaction is the whole motivation for this property, I'd add a line to the doc comment noting it so a user on a slow link knows reducing the part size is the lever. Small thing.

laskoviymishka · 2026-09-17T07:23:53Z

+
+/// Matches Java `S3FileIOProperties.MULTIPART_SIZE_DEFAULT`.
+pub(crate) fn default_multipart_part_size() -> u64 {
+    32 * 1024 * 1024


This default is a bare literal here and repeated in the test (assert_eq!(parse_part_size(None).unwrap(), 32 * 1024 * 1024)), so they can drift apart. I'd pull it into a const DEFAULT_MULTIPART_PART_SIZE: u64 = 32 * 1024 * 1024; next to MULTIPART_PART_SIZE_MIN, have the fn return it, and assert against the const.

laskoviymishka · 2026-09-17T07:23:53Z

+        return Err(Error::new(
+            ErrorKind::DataInvalid,
+            format!(
+                "Invalid {S3_MULTIPART_PART_SIZE_BYTES}: {part_size} is below the S3 minimum part size of {MULTIPART_PART_SIZE_MIN}"


Tiny thing: this prints raw bytes (...minimum part size of 5242880) with no unit, which is hard to read. Something like {part_size} bytes is below the S3 minimum part size of {MULTIPART_PART_SIZE_MIN} (5 MiB) reads a lot friendlier.

laskoviymishka · 2026-09-17T07:23:53Z

+    use iceberg::io::{S3_MULTIPART_PART_SIZE_BYTES, S3_PATH_STYLE_ACCESS};

-    use super::s3_config_parse;
+    use super::*;


Small one — swapping the explicit import for use super::*; pulls every pub(crate) symbol into the test module. I'd keep it explicit (use super::{default_multipart_part_size, s3_config_parse, s3_multipart_part_size_parse};) so the test's dependency surface stays visible.

laskoviymishka · 2026-09-17T07:23:54Z

+            .etag()
+            .expect("MinIO reports an ETag")
+            .trim_matches('"');
+        match etag.rsplit_once('-') {


The <md5>-<N> ETag suffix is a MinIO/plain-S3 detail — with SSE-KMS/C the suffix can be absent, and then this None arm silently returns 1, so a broken multipart upload would read as a passing single-part assertion rather than failing loudly. And parts.parse().unwrap() panics with no context if the segment isn't numeric.

Since the test only runs against MinIO today it's fine in practice, but I'd at least comment the MinIO assumption and expect on the parse so a surprise format fails with a readable message. Not blocking.

YuangGao · 2026-09-18T06:17:06Z

thanks for the review! I have pushed the fixes. The write() question turned out the other way round — details in that thread, short version: write_options() does chunk, and small metadata writes don't pick up multipart. Added two MinIO tests pinning both. Ceiling, const, error units, imports and the ETag note are all in. Two small deviations from the suggestions, noted inline.

laskoviymishka

Almost there. This round did most of what I asked.

The no-op worry I had about the write() path is gone: test_file_io_s3_write_splits_buffer_into_bounded_parts shows it does chunk, and test_file_io_s3_write_below_part_size_stays_single_request closes the other half — small metadata writes stay a single PUT, so commit-heavy workloads don't pick up a 3x round-trip. That's exactly the pair of cases I wanted pinned. One caveat: all of this leans on MinIO, so it only holds if the S3 integration job actually runs on this PR and is green — could you confirm that? I don't want the whole chunking claim resting on a local run.

The one design thing still open is the pub-vs-pub(crate) call on multipart_part_size from last round. Users set the size through the property either way, so I lean pub(crate); if we keep it public I'd mark the variant #[non_exhaustive] so the next S3 option isn't another breaking bump (same thread CTTY raised). Either way is fine — I'd just like a deliberate call.

Everything else I asked for landed:

the 5 GiB upper bound plus the reject-above-max test are in, which also makes the usize::try_from clamp belt-and-suspenders
the parse path validates both the floor and the ceiling with a clean test
public-api.txt regenerated, and anoopj's already through it

Left a couple of small inline notes — the Java doc constant, the 32-bit clamp comment, and a test helper that re-hardcodes creds. Make the pub-field call and confirm those integration tests run in CI, and I'm happy to approve.

laskoviymishka · 2026-09-21T21:28:52Z

 /// Option to skip loading configuration from config file and the env.
 pub const S3_DISABLE_CONFIG_LOAD: &str = "s3.disable-config-load";
+/// Size in bytes of each part of a multipart upload. Must be between 5 MiB and 5 GiB.
+/// Defaults to 32 MiB, matching Java `S3FileIOProperties.MULTIPART_SIZE`.


MULTIPART_SIZE is the property key in Java; the 32 MiB value is MULTIPART_SIZE_DEFAULT. The private const in s3.rs already cites MULTIPART_SIZE_DEFAULT correctly, so this public one should match it.

laskoviymishka · 2026-09-21T21:28:52Z

        config: Arc<S3Config>,
+        /// Bytes carried by one multipart upload request.
+        #[serde(default = "default_multipart_part_size")]
+        multipart_part_size: u64,


This is the pub-vs-pub(crate) call I flagged last round, still open. Since users set the size through the s3.multipart.part-size-bytes property either way, I don't think the field needs to be publicly constructible — pub(crate) would keep the surface smaller.

If we do want it public, two things worth doing while we're here: mark the S3 { } variant #[non_exhaustive] so the next S3 option isn't another fix! bump (CTTY raised the same threading concern), and expose a pub const for the 32 MiB default so a downstream constructor isn't hardcoding 33_554_432. Either direction is fine — I'd just like a deliberate call rather than it defaulting to public. wdyt?

laskoviymishka · 2026-09-21T21:28:52Z

+                // A validated part size is at most 5 GiB, which still exceeds
+                // `usize` on a 32-bit target. Clamp there instead of panicking
+                // on a configured value.
+                chunk: Some(usize::try_from(*multipart_part_size).unwrap_or(usize::MAX)),


Small thing, but the comment stops just short of the actual behavior: on a 32-bit target usize::MAX is ~4 GiB, so a configured 5 GiB part size silently applies as ~4 GiB even though the validator accepted it. Worth a word in the comment so the effective ceiling isn't a surprise. Not blocking.

laskoviymishka · 2026-09-21T21:28:52Z

    async fn write(&self, path: &str, bs: Bytes) -> Result<()> {
        let (op, relative_path) = self.create_operator(&path)?;
-        op.write(relative_path, bs)
+        op.write_options(relative_path, bs, self.write_options())


Last round I wasn't sure this path chunked at all and floated dropping it — test_file_io_s3_write_splits_buffer_into_bounded_parts settles it, it does split. And test_file_io_s3_write_below_part_size_stays_single_request closes the other half of my worry: small metadata writes stay a single PUT, so commit-heavy workloads don't eat a 3x round-trip. Good answer.

One thing I want to be sure of: these assertions all lean on MinIO, so they only mean something if they actually run in CI rather than just locally. Can we confirm the S3 integration job runs on this PR and is green? Once that's confirmed I'm satisfied on this path.

laskoviymishka · 2026-09-21T21:28:52Z

+    async fn upload_part_count(key: &str) -> usize {
+        let mut config = opendal::services::S3Config::default();
+        config.endpoint = Some(get_minio_endpoint());
+        config.access_key_id = Some("admin".to_string());


This helper re-hardcodes the creds and bucket instead of going through get_minio_endpoint() and the shared props. If the fixture ever changes, the stat here fails with a 403 and .expect("MinIO reports an ETag") reports it as a missing ETag — misleading. Could we pull these from the same place the other tests do?

…art-count helper

YuangGao · 2026-09-23T03:01:15Z

thanks for the reviews, updated/confirmed the following:

OpenDalStorage::S3 is now #[non_exhaustive], so the next S3 option isn't another breaking bump. pub(crate) isn't allowed on a variant field (E0449). I skipped the pub const for the default: with the variant closed there's no downstream constructor left to hardcode 33_554_432.
confirmed he integration tests do run in CI. Tests (default) starts MinIO via make docker-up, then cargo nextest run --all-targets --all-features --workspace
The property doc now cites MULTIPART_SIZE_DEFAULT and notes Java's int storage and the sub-2 GB javadoc guidance.
The 32-bit comment spells out the effective usize::MAX ceiling.
upload_part_count shares creds, region and bucket with the other tests, and now carries the RetryLayer the storage under test uses.

@laskoviymishka @CTTY ready for another look when you have a chance

fix(io): bound S3 write request size with a configurable part size

084f3b6

YuangGao mentioned this pull request Sep 9, 2026

opendal FileIO hard-wires TimeoutLayer defaults (10s per-IO) with no way to configure them; large Parquet row groups fail deterministically on S3 #2977

Open

YuangGao marked this pull request as ready for review September 9, 2026 06:47

anoopj reviewed Sep 9, 2026

View reviewed changes

fix(io): store multipart part size as u64 and refresh public API snap…

54fadc1

…shots

anoopj approved these changes Sep 16, 2026

View reviewed changes

CTTY reviewed Sep 16, 2026

View reviewed changes

laskoviymishka requested changes Sep 17, 2026

View reviewed changes

fix(io): validate S3 part size ceiling and cover the one-shot write path

cce58b9

YuangGao changed the title ~~fix(io): bound S3 write request size with a configurable part size~~ fix!(io): bound S3 write request size with a configurable part size Sep 18, 2026

laskoviymishka requested changes Sep 22, 2026

View reviewed changes

This was referenced Sep 22, 2026

feat(storage-opendal)!: make the per-IO-operation timeout configurable #3263

Open

Iceberg scan fails with a hard-wired 10s OpenDAL io_timeout that cannot be configured apache/datafusion-comet#6124

Open

fix(io): mark the S3 variant non_exhaustive and stabilize the MinIO p…

88622a0

…art-count helper

Conversation

YuangGao commented Sep 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue does this PR close?

What changes are included in this PR?

Are these changes tested?

AI Disclosure

Uh oh!

anoopj left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

YuangGao commented Sep 16, 2026

Uh oh!

anoopj left a comment

Choose a reason for hiding this comment

Uh oh!

CTTY left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laskoviymishka left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

YuangGao commented Sep 18, 2026

Uh oh!

laskoviymishka left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

YuangGao commented Sep 23, 2026

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

YuangGao commented Sep 9, 2026 •

edited

Loading