Ensure all non-final multipart uploads in filesystem sink are the same size - #889
Merged
Conversation
cmackenzie1
approved these changes
May 16, 2025
cmackenzie1
left a comment
Collaborator
There was a problem hiding this comment.
Overall LGTM but I'm still getting up to speed on the rest of the codebase.
Once other quirk about R2 is the minimum part size is 5 MiB. Does the multipart stuff need to be aware of that limitation or at least enforce target_part_size is >= 5 MiB when destination is R2?
Member
Author
|
S3 actually has the same limitation, so I've modified the configuration to enforce that target_part_size is at least 5MB. |
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
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.
R2 requires that all parts of a multipart upload (aside from the last one) be the same size. This PR changes how the pieces of the FilesystemSink work together to meet that requirement, while also simplifying the logic around multipart handling.
The FilesystemSink has a few major traits, which each have several implementations to abstract over json/parquet and object store/local filesystem:
Previously, the BatchBufferingWriters were "multipart aware"—they were responsible for deciding when to break up a batch into a new multipart write. For parquet, this would happen in the following process:
This PR changes the responsibilities such that it's more cleanly layered. The BatchBufferingWriter is now responsible only for converting its format and buffering, and has no awareness of multipart uploads. The BatchMultipartWriter decides when and how much to upload for a particular part. The new process is:
a. If we don't have an active multipart upload, we check if the buffer is larger than our desired multipart upload size
b. If we do have one, we check if it's larger than the actual multipart upload size for this upload
A bit of additional complexity comes in once we're ready to finish the multipart upload, as we need to ensure that the final part is smaller than our part size. Before sending the last part we check if that's the case, and if not we split it into two parts.
In addition to the functional changes, there are also some performance improvements from moving from Vec to Bytes/BytesMut, which allows us to avoid some copies.
Breaking changes
This PR also includes some breaking changes to how we configure filesystem options; these are aimed at making the configuration more useful and easier to understand:
parquet_row_batch_sizeis removed (this was not actually be used anywhere)parquet_row_group_sizeis replaced withparquet.row_group_size_bytes, as I believe size-based configuration is more useful than row-count base configurationparquet_compressionis renamedparquet.compressionto match our standard option styleTesting
These changes were tested against R2 and Minio, with parquet, json, and deltalake. To ensure that checkpointing correctness is unaffected, I used the following query:
which produces an incrementing counter (along with junk data to run up the file size). While that was running, I stopped/started and randomly kill -9'd the worker and controller processes and allowed it to recover. Then, I verified that the data was consistent with this duckdb query:
which should produce the same number for each (the +1 on the max is due to 0-based indexing on the counter)