feat(writer): Support ObjectStorageLocationGenerator - #2871
Conversation
hsiang-c
changed the title
|
@CTTY If you could take a look when you have time, thank you! |
CTTY
left a comment
CTTY
left a comment
There was a problem hiding this comment.
Just did a quick review, LGTM overall! Left some minor comments
| } | ||
|
|
||
| /// Strip a single trailing slash from a location, if present. | ||
| fn strip_trailing_slash(location: &str) -> &str { |
There was a problem hiding this comment.
in table_properties.rs, there is already one strip_trailing_slash, can we refactor and use that? We can create a new iceberg/src/util/location.rs to store such helpers
There was a problem hiding this comment.
Good point, I moved the helper function to util/location.rs.
| @@ -45,6 +45,19 @@ const WRITE_DATA_LOCATION: &str = "write.data.path"; | |||
| const WRITE_FOLDER_STORAGE_LOCATION: &str = "write.folder-storage.path"; | |||
| const DEFAULT_DATA_DIR: &str = "/data"; | |||
|
|
|||
| /// Deprecated object storage path property, kept as a fallback for compatibility. | |||
| const WRITE_OBJECT_STORAGE_LOCATION: &str = "write.object-storage.path"; | |||
| /// Property controlling whether partition values are included in object storage paths. | |||
| const WRITE_OBJECT_STORAGE_PARTITIONED_PATHS: &str = "write.object-storage.partitioned-paths"; | |||
| const WRITE_OBJECT_STORAGE_PARTITIONED_PATHS_DEFAULT: bool = true; | |||
|
|
|||
| /// Number of trailing hash bits used to build the entropy directories. | |||
| const HASH_BINARY_STRING_BITS: usize = 20; | |||
| /// Length of each entropy directory. | |||
| const ENTROPY_DIR_LENGTH: usize = 4; | |||
| /// Number of entropy directories generated from the hash. | |||
| const ENTROPY_DIR_DEPTH: usize = 3; | |||
There was a problem hiding this comment.
We should move all of these to table_properties.rs
| let hash_code = murmur3::murmur3_32(&mut bytes, 0).unwrap(); | ||
|
|
||
| // Force the top bit so the binary string always has 32 characters (Rust, like Java's | ||
| // Integer.toBinaryString, drops leading zeros otherwise), then keep the trailing bits. |
There was a problem hiding this comment.
I think {:032b} already forces the binary to be 32 characters with zero paddings? In java we have to do | 0x8000_0000 because the hash to binary string conversion won't preserve the leading zeroes, but here looks like we don't actually need to set the first bit to 1?
The logic is ok, but I think it's worth clarifying in the comment that we are setting the first bit to 1 simply to imitate java's padding logic
There was a problem hiding this comment.
Good catch, this is indeed redundant and I removed it.
aa0efec to
0522dce
Compare
| // under the License. | ||
|
|
||
| /// Strips trailing slashes from a location, preserving a bare URI scheme root | ||
| pub(crate) fn strip_trailing_slash(path: &str) -> &str { |
There was a problem hiding this comment.
There is a unit test in table_properties.rs for this function, could you move that here as well?
|
|
||
| use std::num::NonZeroUsize; | ||
|
|
||
| pub(crate) mod location; |
| /// Deprecated object storage path property, kept as a fallback for compatibility, | ||
| pub write_object_storage_location: Option<String>, | ||
| /// Whether partition values are included in object storage paths. | ||
| pub write_object_storage_partitioned_paths: Option<String>, |
| <T as FromStr>::Err: Display, | ||
| { | ||
| properties.get(key).map_or(Ok(default), |value| { | ||
| value.parse::<T>().map_err(|e| { |
There was a problem hiding this comment.
I just realized that rust parse::<bool> will only accept lower case "true" or "false", this will cause valid configuration on the java side like write.object-storage.partitioned-paths=False auto fall back to default(true) on the rust side.
We should fix this by adding a new parse_property_bool and add necessary comments, also need to migrate the existing parse::<bool> to use the new function. we can do it in a follow up PR tho
| let include_partition_paths = prop | ||
| .get(TableProperties::PROPERTY_WRITE_OBJECT_STORAGE_PARTITIONED_PATHS) | ||
| .and_then(|value| value.parse::<bool>().ok()) | ||
| .unwrap_or(TableProperties::PROPERTY_WRITE_OBJECT_STORAGE_PARTITIONED_PATHS_DEFAULT); |
There was a problem hiding this comment.
We should not parse the value here, let's use tableProperties::parse_property
2 participants
Which issue does this PR close?
What changes are included in this PR?
ObjectStorageLocationGenerator, another implementation toLocationGeneratorthat generates a deterministic hash for each stored file and appends the hash directly after thewrite.data.path. The equivalent class in Iceberg Java is ObjectStoreLocationProviderObjectStorageLocationGeneratorconfigurable yet. Plan to do it in the follow-up PR.Are these changes tested?
Unit tests