CTTY commented on code in PR #3179:
URL: https://github.com/apache/iceberg-rust/pull/3179#discussion_r4077197028
##########
crates/storage/opendal/src/s3.rs:
##########
@@ -36,6 +36,36 @@ use url::Url;
use crate::utils::{from_opendal_error, is_truthy};
+/// S3 rejects a non-final part smaller than this.
+const MULTIPART_PART_SIZE_MIN: u64 = 5 * 1024 * 1024;
+
+/// Matches Java `S3FileIOProperties.MULTIPART_SIZE_DEFAULT`.
+pub(crate) fn default_multipart_part_size() -> u64 {
+ 32 * 1024 * 1024
+}
+
+/// Parse iceberg props to s3 multipart upload part size.
+pub(crate) fn s3_multipart_part_size_parse(m: &HashMap<String, String>) ->
Result<u64> {
+ let Some(value) = m.get(S3_MULTIPART_PART_SIZE_BYTES) else {
+ return Ok(default_multipart_part_size());
+ };
+ let part_size = value.parse::<u64>().map_err(|e| {
+ Error::new(
+ ErrorKind::DataInvalid,
+ format!("Invalid {S3_MULTIPART_PART_SIZE_BYTES}: {value}: {e}"),
+ )
+ })?;
+ if part_size < MULTIPART_PART_SIZE_MIN {
Review Comment:
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
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
