hsiang-c commented on code in PR #2871:
URL: https://github.com/apache/iceberg-rust/pull/2871#discussion_r3692073693
##########
crates/iceberg/src/writer/file_writer/location_generator.rs:
##########
@@ -93,6 +106,153 @@ impl LocationGenerator for DefaultLocationGenerator {
}
}
+/// `ObjectStorageLocationGenerator` injects hash entropy into generated file
locations so that
+/// files are spread across many object-store prefixes.
+///
+/// Object stores such as S3 shard request throughput by key prefix, so
writing every file under a
+/// common `.../data/` prefix creates a throughput hotspot. This generator
prepends a
+/// deterministic, hashed directory tree (derived from the file name) to each
location, mirroring
+/// Java Iceberg's `ObjectStoreLocationProvider`.
+///
+/// The behavior is controlled by these table properties:
+/// * `write.data.path` / `write.object-storage.path` /
`write.folder-storage.path` - the base data
+/// location (checked in that order), defaulting to `{table_location}/data`.
+/// * `write.object-storage.partitioned-paths` - whether partition values are
included in the path
+/// (defaults to `true`).
+#[derive(Clone, Debug)]
+pub struct ObjectStorageLocationGenerator {
+ storage_location: String,
+ /// Database/table context, only set when the storage location is outside
the table location.
+ context: Option<String>,
+ include_partition_paths: bool,
+}
+
+impl ObjectStorageLocationGenerator {
+ /// Create a new `ObjectStorageLocationGenerator` from table metadata.
+ pub fn new(table_metadata: &TableMetadata) -> Result<Self> {
+ let table_location = strip_trailing_slash(table_metadata.location());
+ let prop = table_metadata.properties();
+ let storage_location =
+ strip_trailing_slash(&resolve_data_location(prop,
table_location)).to_string();
+
+ // If the storage location is within the table prefix, files are
already scoped to this
+ // table so there is no need to add database/table context to avoid
collisions.
+ let context = if storage_location.starts_with(table_location) {
+ None
+ } else {
+ Some(path_context(table_location))
+ };
+
+ let include_partition_paths = prop
+ .get(WRITE_OBJECT_STORAGE_PARTITIONED_PATHS)
+ .and_then(|value| value.parse::<bool>().ok())
+ .unwrap_or(WRITE_OBJECT_STORAGE_PARTITIONED_PATHS_DEFAULT);
+
+ Ok(Self {
+ storage_location,
+ context,
+ include_partition_paths,
+ })
+ }
+
+ /// Build the final location for a fully-formed data file name (which may
already include a
+ /// partition path).
+ fn new_data_location(&self, name: &str) -> String {
+ let hash = self.compute_hash(name);
+ if let Some(context) = &self.context {
+ format!("{}/{}/{}/{}", self.storage_location, hash, context, name)
+ } else if self.include_partition_paths {
+ format!("{}/{}/{}", self.storage_location, hash, name)
+ } else {
+ // When partition paths are excluded, join the entropy to the file
name with `-` so the
+ // file still lives directly under the storage location.
+ format!("{}/{}-{}", self.storage_location, hash, name)
+ }
+ }
+
+ /// Compute the entropy directory tree for the given name, e.g.
`0101/0110/1001/10110010`.
+ fn compute_hash(&self, name: &str) -> String {
+ let mut bytes = name.as_bytes();
+ 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.
Review Comment:
Good catch, this is indeed redundant and I removed it.
--
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]
