From 7a2a71e0c448427d49fbf1e8f91e86931ba40f6b Mon Sep 17 00:00:00 2001 From: huanghsiang_cheng Date: Mon, 20 Jul 2026 15:22:07 -0700 Subject: [PATCH 01/11] Add ObjectStorageLocationGenerator --- Cargo.lock | 1 + crates/iceberg/Cargo.toml | 1 + .../writer/file_writer/location_generator.rs | 371 +++++++++++++++++- 3 files changed, 371 insertions(+), 2 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index d7660914a3..5a1d50c2fa 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3808,6 +3808,7 @@ dependencies = [ "typed-builder", "typetag", "url", + "urlencoding", "uuid", "zeroize", "zstd", diff --git a/crates/iceberg/Cargo.toml b/crates/iceberg/Cargo.toml index 66eadfb7ec..a0a9502fc4 100644 --- a/crates/iceberg/Cargo.toml +++ b/crates/iceberg/Cargo.toml @@ -83,6 +83,7 @@ url = { workspace = true } uuid = { workspace = true } zeroize = { workspace = true } zstd = { workspace = true } +urlencoding = "2.1.3" [dev-dependencies] expect-test = { workspace = true } diff --git a/crates/iceberg/src/writer/file_writer/location_generator.rs b/crates/iceberg/src/writer/file_writer/location_generator.rs index d37d00c936..9ce9d1b0e0 100644 --- a/crates/iceberg/src/writer/file_writer/location_generator.rs +++ b/crates/iceberg/src/writer/file_writer/location_generator.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; + #[derive(Clone, Debug)] /// `DefaultLocationGenerator` used to generate the data dir location of data file. /// The location is generated based on the table location and the data location in table properties. @@ -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, + include_partition_paths: bool, +} + +impl ObjectStorageLocationGenerator { + /// Create a new `ObjectStorageLocationGenerator` from table metadata. + pub fn new(table_metadata: &TableMetadata) -> Result { + 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::().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. + let binary = format!("{:032b}", hash_code | 0x8000_0000); + let hash = &binary[binary.len() - HASH_BINARY_STRING_BITS..]; + dirs_from_hash(hash) + } +} + +impl LocationGenerator for ObjectStorageLocationGenerator { + fn generate_location(&self, partition_key: Option<&PartitionKey>, file_name: &str) -> String { + let name = + if self.include_partition_paths && !PartitionKey::is_effectively_none(partition_key) { + format!("{}/{}", partition_key.unwrap().to_path(), file_name) + } else { + file_name.to_string() + }; + self.new_data_location(&name) + } +} + +/// Strip a single trailing slash from a location, if present. +fn strip_trailing_slash(location: &str) -> &str { + location.strip_suffix('/').unwrap_or(location) +} + +/// Derive the `{parent}/{name}` context from a table location, mirroring Hadoop's +/// `Path.getParent().getName()` / `Path.getName()`. +fn path_context(table_location: &str) -> String { + let mut segments = table_location.rsplit('/').filter(|s| !s.is_empty()); + let name = segments.next().unwrap_or(""); + match segments.next() { + Some(parent) => format!("{parent}/{name}"), + None => name.to_string(), + } +} + +/// Divide a binary hash string into directories for optimized listing/orphan removal. +/// +/// With `ENTROPY_DIR_DEPTH = 3` and `ENTROPY_DIR_LENGTH = 4`, the 20-bit hash +/// `10011001100110011001` becomes `1001/1001/1001/10011001`. +fn dirs_from_hash(hash: &str) -> String { + let mut result = String::new(); + + let mut i = 0; + while i < ENTROPY_DIR_DEPTH * ENTROPY_DIR_LENGTH { + if i > 0 { + result.push('/'); + } + let end = (i + ENTROPY_DIR_LENGTH).min(hash.len()); + result.push_str(&hash[i..end]); + i += ENTROPY_DIR_LENGTH; + } + + if hash.len() > ENTROPY_DIR_DEPTH * ENTROPY_DIR_LENGTH { + result.push('/'); + result.push_str(&hash[ENTROPY_DIR_DEPTH * ENTROPY_DIR_LENGTH..]); + } + + result +} + +/// Resolve the base data location from table properties, falling back to `{table_location}/data`. +/// +/// Precedence follows Java Iceberg: `write.data.path` first, then the deprecated +/// `write.object-storage.path` and `write.folder-storage.path` properties, and finally the default +/// `{table_location}/data`. +fn resolve_data_location( + properties: &std::collections::HashMap, + table_location: &str, +) -> String { + properties + .get(WRITE_DATA_LOCATION) + .or_else(|| properties.get(WRITE_OBJECT_STORAGE_LOCATION)) + .or_else(|| properties.get(WRITE_FOLDER_STORAGE_LOCATION)) + .cloned() + .unwrap_or_else(|| format!("{table_location}{DEFAULT_DATA_DIR}")) +} + /// `FileNameGeneratorTrait` used to generate file name for data file. The file name can be passed to `LocationGenerator` to generate the location of the file. pub trait FileNameGenerator: Clone + Send + Sync + 'static { /// Generate a file name. @@ -153,10 +313,40 @@ pub(crate) mod test { Struct, StructType, TableMetadata, Transform, Type, }; use crate::writer::file_writer::location_generator::{ - DefaultLocationGenerator, FileNameGenerator, WRITE_DATA_LOCATION, - WRITE_FOLDER_STORAGE_LOCATION, + DefaultLocationGenerator, FileNameGenerator, ObjectStorageLocationGenerator, + WRITE_DATA_LOCATION, WRITE_FOLDER_STORAGE_LOCATION, WRITE_OBJECT_STORAGE_PARTITIONED_PATHS, }; + /// Build a minimal `TableMetadata` for location generator tests. + fn table_metadata_with(location: &str, properties: HashMap) -> TableMetadata { + TableMetadata { + format_version: FormatVersion::V2, + table_uuid: Uuid::parse_str("fb072c92-a02b-11e9-ae9c-1bb7bc9eca94").unwrap(), + location: location.to_string(), + last_updated_ms: 1515100955770, + last_column_id: 1, + schemas: HashMap::new(), + current_schema_id: 1, + partition_specs: HashMap::new(), + default_spec: PartitionSpec::unpartition_spec().into(), + default_partition_type: StructType::new(vec![]), + last_partition_id: 1000, + default_sort_order_id: 0, + sort_orders: HashMap::from_iter(vec![]), + snapshots: HashMap::default(), + current_snapshot_id: None, + last_sequence_number: 1, + properties, + snapshot_log: Vec::new(), + metadata_log: vec![], + refs: HashMap::new(), + statistics: HashMap::new(), + partition_statistics: HashMap::new(), + encryption_keys: HashMap::new(), + next_row_id: 0, + } + } + #[test] fn test_default_location_generate() { let mut table_metadata = TableMetadata { @@ -339,4 +529,181 @@ pub(crate) mod test { next_row_id: 0, } } + + #[test] + fn test_object_storage_hash_injection() { + // Golden vectors ported from Java's TestLocationProvider#testHashInjection, verifying that + // the murmur3 entropy directories match Java Iceberg exactly. + let table_metadata = table_metadata_with("s3://data.db/table", HashMap::new()); + let location_gen = ObjectStorageLocationGenerator::new(&table_metadata).unwrap(); + + for (file_name, expected) in [ + ("a", "s3://data.db/table/data/0101/0110/1001/10110010/a"), + ("b", "s3://data.db/table/data/1110/0111/1110/00000011/b"), + ("c", "s3://data.db/table/data/0010/1101/0110/01011111/c"), + ("d", "s3://data.db/table/data/1001/0001/0100/01110011/d"), + ] { + assert_eq!(location_gen.generate_location(None, file_name), expected); + } + } + + #[test] + fn test_object_storage_include_partition_paths() { + // With partitioned paths enabled (the default), the partition path is part of the hashed + // name and appears after the entropy directories. + let schema = Arc::new( + Schema::builder() + .with_schema_id(1) + .with_fields(vec![ + NestedField::required(1, "id", Type::Primitive(PrimitiveType::Int)).into(), + NestedField::required(2, "a", Type::Primitive(PrimitiveType::String)).into(), + ]) + .build() + .unwrap(), + ); + let partition_spec = PartitionSpec::builder(schema.clone()) + .add_partition_field("id", "id", Transform::Identity) + .unwrap() + .add_partition_field("a", "a_trunc_3", Transform::Truncate(3)) + .unwrap() + .build() + .unwrap(); + let partition_data = + Struct::from_iter([Some(Literal::int(0)), Some(Literal::string("apa"))]); + let partition_key = PartitionKey::new(partition_spec, schema, partition_data); + + let table_metadata = table_metadata_with("s3://data.db/table", HashMap::new()); + let location_gen = ObjectStorageLocationGenerator::new(&table_metadata).unwrap(); + let location = location_gen.generate_location(Some(&partition_key), "test.parquet"); + + assert!( + location.starts_with("s3://data.db/table/data/"), + "unexpected location: {location}" + ); + // The partition path is included + assert!( + location.ends_with("/id=0/a_trunc_3=apa/test.parquet"), + "unexpected location: {location}" + ); + } + + /// See: https://github.com/apache/iceberg/pull/10329 + #[test] + fn test_object_storage_include_partition_paths_with_special_characters() { + let schema = Arc::new( + Schema::builder() + .with_schema_id(1) + .with_fields(vec![ + NestedField::required(1, "data#1", Type::Primitive(PrimitiveType::Int)).into(), + ]) + .build() + .unwrap(), + ); + let partition_spec = PartitionSpec::builder(schema.clone()) + .add_partition_field("data#1", "data#1", Transform::Identity) + .unwrap() + .build() + .unwrap(); + let partition_data = Struct::from_iter([Some(Literal::string("val#1"))]); + let partition_key = PartitionKey::new(partition_spec, schema, partition_data); + + let table_metadata = table_metadata_with( + "s3://data.db/table", + HashMap::from([( + WRITE_OBJECT_STORAGE_PARTITIONED_PATHS.to_string(), + "true".to_string(), + )]), + ); + let location_gen = ObjectStorageLocationGenerator::new(&table_metadata).unwrap(); + let location = location_gen.generate_location(Some(&partition_key), "test.parquet"); + + assert_eq!( + location, + "s3://data.db/table/data/0000/1011/0110/00001000/data%231=val%231/test.parquet" + ); + } + + #[test] + fn test_object_storage_exclude_partition_paths() { + // Golden vector ported from Java's TestLocationProvider#testExcludePartitionInPath. With + // partitioned paths disabled, the partition value is dropped and the last entropy dir is + // joined to the file name with `-`. + let schema = Arc::new( + Schema::builder() + .with_schema_id(1) + .with_fields(vec![ + NestedField::required(1, "id", Type::Primitive(PrimitiveType::Int)).into(), + ]) + .build() + .unwrap(), + ); + let partition_spec = PartitionSpec::builder(schema.clone()) + .add_partition_field("id", "id", Transform::Identity) + .unwrap() + .build() + .unwrap(); + let partition_data = Struct::from_iter([Some(Literal::int(0))]); + let partition_key = PartitionKey::new(partition_spec, schema, partition_data); + + let table_metadata = table_metadata_with( + "s3://data.db/table", + HashMap::from([( + WRITE_OBJECT_STORAGE_PARTITIONED_PATHS.to_string(), + "false".to_string(), + )]), + ); + let location_gen = ObjectStorageLocationGenerator::new(&table_metadata).unwrap(); + let location = location_gen.generate_location(Some(&partition_key), "test.parquet"); + + assert_eq!( + location, + "s3://data.db/table/data/0110/1010/0011/11101000-test.parquet" + ); + } + + #[test] + fn test_object_storage_context_when_data_location_outside_table() { + // When the data location is outside the table location, the database/table context is + // injected after the entropy directories to avoid cross-table collisions. + let table_metadata = table_metadata_with( + "s3://data.db/table", + HashMap::from([( + WRITE_DATA_LOCATION.to_string(), + "s3://custom-bucket/objects".to_string(), + )]), + ); + let location_gen = ObjectStorageLocationGenerator::new(&table_metadata).unwrap(); + let location = location_gen.generate_location(None, "a"); + + // Entropy for "a" is 0101/0110/1001/10110010, then the "data.db/table" context, then file. + assert_eq!( + location, + "s3://custom-bucket/objects/0101/0110/1001/10110010/data.db/table/a" + ); + } + + #[test] + fn test_object_storage_data_location_precedence() { + // write.data.path takes precedence over the deprecated folder-storage fallback. + let table_metadata = table_metadata_with( + "s3://data.db/table", + HashMap::from([ + ( + WRITE_DATA_LOCATION.to_string(), + "s3://data.db/table/data_primary".to_string(), + ), + ( + WRITE_FOLDER_STORAGE_LOCATION.to_string(), + "s3://data.db/table/data_legacy".to_string(), + ), + ]), + ); + let location_gen = ObjectStorageLocationGenerator::new(&table_metadata).unwrap(); + let location = location_gen.generate_location(None, "a"); + + assert_eq!( + location, + "s3://data.db/table/data_primary/0101/0110/1001/10110010/a" + ); + } } From 6c5086397a6129407bc08da594443c9a46653ee8 Mon Sep 17 00:00:00 2001 From: huanghsiang_cheng Date: Tue, 21 Jul 2026 13:39:32 -0700 Subject: [PATCH 02/11] Fix TOML format --- crates/iceberg/Cargo.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/iceberg/Cargo.toml b/crates/iceberg/Cargo.toml index a0a9502fc4..26c4d48717 100644 --- a/crates/iceberg/Cargo.toml +++ b/crates/iceberg/Cargo.toml @@ -80,10 +80,10 @@ tracing = { workspace = true } typed-builder = { workspace = true } typetag = { workspace = true } url = { workspace = true } +urlencoding = "2.1.3" uuid = { workspace = true } zeroize = { workspace = true } zstd = { workspace = true } -urlencoding = "2.1.3" [dev-dependencies] expect-test = { workspace = true } From 8341f878f44ac3bfd6b42edc83e960dbe0fb5321 Mon Sep 17 00:00:00 2001 From: huanghsiang_cheng Date: Tue, 21 Jul 2026 13:45:20 -0700 Subject: [PATCH 03/11] Update public API --- crates/iceberg/public-api.txt | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/crates/iceberg/public-api.txt b/crates/iceberg/public-api.txt index 12f844b362..73a453c594 100644 --- a/crates/iceberg/public-api.txt +++ b/crates/iceberg/public-api.txt @@ -3245,6 +3245,15 @@ impl core::fmt::Debug for iceberg::writer::file_writer::location_generator::Defa pub fn iceberg::writer::file_writer::location_generator::DefaultLocationGenerator::fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result impl iceberg::writer::file_writer::location_generator::LocationGenerator for iceberg::writer::file_writer::location_generator::DefaultLocationGenerator pub fn iceberg::writer::file_writer::location_generator::DefaultLocationGenerator::generate_location(&self, partition_key: core::option::Option<&iceberg::spec::PartitionKey>, file_name: &str) -> alloc::string::String +pub struct iceberg::writer::file_writer::location_generator::ObjectStorageLocationGenerator +impl iceberg::writer::file_writer::location_generator::ObjectStorageLocationGenerator +pub fn iceberg::writer::file_writer::location_generator::ObjectStorageLocationGenerator::new(table_metadata: &iceberg::spec::TableMetadata) -> iceberg::Result +impl core::clone::Clone for iceberg::writer::file_writer::location_generator::ObjectStorageLocationGenerator +pub fn iceberg::writer::file_writer::location_generator::ObjectStorageLocationGenerator::clone(&self) -> iceberg::writer::file_writer::location_generator::ObjectStorageLocationGenerator +impl core::fmt::Debug for iceberg::writer::file_writer::location_generator::ObjectStorageLocationGenerator +pub fn iceberg::writer::file_writer::location_generator::ObjectStorageLocationGenerator::fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result +impl iceberg::writer::file_writer::location_generator::LocationGenerator for iceberg::writer::file_writer::location_generator::ObjectStorageLocationGenerator +pub fn iceberg::writer::file_writer::location_generator::ObjectStorageLocationGenerator::generate_location(&self, partition_key: core::option::Option<&iceberg::spec::PartitionKey>, file_name: &str) -> alloc::string::String pub trait iceberg::writer::file_writer::location_generator::FileNameGenerator: core::clone::Clone + core::marker::Send + core::marker::Sync + 'static pub fn iceberg::writer::file_writer::location_generator::FileNameGenerator::generate_file_name(&self) -> alloc::string::String impl iceberg::writer::file_writer::location_generator::FileNameGenerator for iceberg::writer::file_writer::location_generator::DefaultFileNameGenerator @@ -3253,6 +3262,8 @@ pub trait iceberg::writer::file_writer::location_generator::LocationGenerator: c pub fn iceberg::writer::file_writer::location_generator::LocationGenerator::generate_location(&self, partition_key: core::option::Option<&iceberg::spec::PartitionKey>, file_name: &str) -> alloc::string::String impl iceberg::writer::file_writer::location_generator::LocationGenerator for iceberg::writer::file_writer::location_generator::DefaultLocationGenerator pub fn iceberg::writer::file_writer::location_generator::DefaultLocationGenerator::generate_location(&self, partition_key: core::option::Option<&iceberg::spec::PartitionKey>, file_name: &str) -> alloc::string::String +impl iceberg::writer::file_writer::location_generator::LocationGenerator for iceberg::writer::file_writer::location_generator::ObjectStorageLocationGenerator +pub fn iceberg::writer::file_writer::location_generator::ObjectStorageLocationGenerator::generate_location(&self, partition_key: core::option::Option<&iceberg::spec::PartitionKey>, file_name: &str) -> alloc::string::String pub mod iceberg::writer::file_writer::rolling_writer pub struct iceberg::writer::file_writer::rolling_writer::RollingFileWriter impl iceberg::writer::file_writer::rolling_writer::RollingFileWriter where B: iceberg::writer::file_writer::FileWriterBuilder, L: iceberg::writer::file_writer::location_generator::LocationGenerator, F: iceberg::writer::file_writer::location_generator::FileNameGenerator From e094fb6efe27cff6260c86e652e8959ce8bf0a2c Mon Sep 17 00:00:00 2001 From: huanghsiang_cheng Date: Thu, 30 Jul 2026 13:39:48 -0700 Subject: [PATCH 04/11] Revert encoding fix and disable a test for now --- Cargo.lock | 1 - crates/iceberg/Cargo.toml | 1 - crates/iceberg/src/writer/file_writer/location_generator.rs | 2 +- 3 files changed, 1 insertion(+), 3 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 5a1d50c2fa..d7660914a3 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3808,7 +3808,6 @@ dependencies = [ "typed-builder", "typetag", "url", - "urlencoding", "uuid", "zeroize", "zstd", diff --git a/crates/iceberg/Cargo.toml b/crates/iceberg/Cargo.toml index 26c4d48717..66eadfb7ec 100644 --- a/crates/iceberg/Cargo.toml +++ b/crates/iceberg/Cargo.toml @@ -80,7 +80,6 @@ tracing = { workspace = true } typed-builder = { workspace = true } typetag = { workspace = true } url = { workspace = true } -urlencoding = "2.1.3" uuid = { workspace = true } zeroize = { workspace = true } zstd = { workspace = true } diff --git a/crates/iceberg/src/writer/file_writer/location_generator.rs b/crates/iceberg/src/writer/file_writer/location_generator.rs index 9ce9d1b0e0..983f4f562d 100644 --- a/crates/iceberg/src/writer/file_writer/location_generator.rs +++ b/crates/iceberg/src/writer/file_writer/location_generator.rs @@ -587,7 +587,7 @@ pub(crate) mod test { ); } - /// See: https://github.com/apache/iceberg/pull/10329 + #[ignore] #[test] fn test_object_storage_include_partition_paths_with_special_characters() { let schema = Arc::new( From d3bf246d0feca0f5fe40c1f00ab6668b72fafcc5 Mon Sep 17 00:00:00 2001 From: huanghsiang_cheng Date: Thu, 30 Jul 2026 16:24:29 -0700 Subject: [PATCH 05/11] Move object storage properties to table_properties.rs --- crates/iceberg/src/spec/table_properties.rs | 33 +++++++++++++ .../writer/file_writer/location_generator.rs | 49 +++++++------------ 2 files changed, 52 insertions(+), 30 deletions(-) diff --git a/crates/iceberg/src/spec/table_properties.rs b/crates/iceberg/src/spec/table_properties.rs index 379feee5c1..5bc209b7b3 100644 --- a/crates/iceberg/src/spec/table_properties.rs +++ b/crates/iceberg/src/spec/table_properties.rs @@ -173,6 +173,14 @@ pub struct TableProperties { pub encryption_key_id: Option, /// The encryption data encryption key length in bytes. pub encryption_data_key_length: usize, + /// Base directory for data files + pub write_data_location: Option, + /// Deprecated and will be removed in Iceberg Java, use [write_data_location] instead. + pub write_folder_storage_location: Option, + /// Deprecated object storage path property, kept as a fallback for compatibility, + pub write_object_storage_location: Option, + /// Whether partition values are included in object storage paths. + pub write_object_storage_partitioned_paths: Option, } impl TableProperties { @@ -325,6 +333,19 @@ impl TableProperties { pub const PROPERTY_ENCRYPTION_DATA_KEY_LENGTH: &str = "encryption.data-key-length"; /// Default value for the encryption DEK length (16 bytes = AES-128). pub const PROPERTY_ENCRYPTION_DATA_KEY_LENGTH_DEFAULT: usize = 16; + /// Property key for the base directory for data files + pub const PROPERTY_WRITE_DATA_LOCATION: &str = "write.data.path"; + /// Property key for deprecated [write_folder_storage_location] + pub const PROPERTY_WRITE_FOLDER_STORAGE_LOCATION: &str = "write.folder-storage.path"; + /// Default directory for data files + pub const DEFAULT_DATA_DIR: &str = "/data"; + /// Property key for deprecated object storage path, kept as a fallback for compatibility. + pub const PROPERTY_WRITE_OBJECT_STORAGE_LOCATION: &str = "write.object-storage.path"; + /// Property key for controlling whether partition values are included in object storage paths. + pub const PROPERTY_WRITE_OBJECT_STORAGE_PARTITIONED_PATHS: &str = + "write.object-storage.partitioned-paths"; + /// Default value for [PROPERTY_WRITE_OBJECT_STORAGE_PARTITIONED_PATHS] + pub const WRITE_OBJECT_STORAGE_PARTITIONED_PATHS_DEFAULT: bool = true; } impl TryFrom<&HashMap> for TableProperties { @@ -421,6 +442,18 @@ impl TryFrom<&HashMap> for TableProperties { TableProperties::PROPERTY_ENCRYPTION_DATA_KEY_LENGTH, TableProperties::PROPERTY_ENCRYPTION_DATA_KEY_LENGTH_DEFAULT, )?, + write_data_location: props + .get(TableProperties::PROPERTY_WRITE_DATA_LOCATION) + .cloned(), + write_folder_storage_location: props + .get(TableProperties::PROPERTY_WRITE_FOLDER_STORAGE_LOCATION) + .cloned(), + write_object_storage_location: props + .get(TableProperties::PROPERTY_WRITE_OBJECT_STORAGE_LOCATION) + .cloned(), + write_object_storage_partitioned_paths: props + .get(TableProperties::PROPERTY_WRITE_OBJECT_STORAGE_PARTITIONED_PATHS) + .cloned(), }) } } diff --git a/crates/iceberg/src/writer/file_writer/location_generator.rs b/crates/iceberg/src/writer/file_writer/location_generator.rs index 983f4f562d..01295d4ab5 100644 --- a/crates/iceberg/src/writer/file_writer/location_generator.rs +++ b/crates/iceberg/src/writer/file_writer/location_generator.rs @@ -21,7 +21,7 @@ use std::sync::Arc; use std::sync::atomic::AtomicU64; use crate::Result; -use crate::spec::{DataFileFormat, PartitionKey, TableMetadata}; +use crate::spec::{DataFileFormat, PartitionKey, TableMetadata, TableProperties}; /// `LocationGenerator` used to generate the location of data file. pub trait LocationGenerator: Clone + Send + Sync + 'static { @@ -41,16 +41,6 @@ pub trait LocationGenerator: Clone + Send + Sync + 'static { fn generate_location(&self, partition_key: Option<&PartitionKey>, file_name: &str) -> String; } -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. @@ -71,12 +61,12 @@ impl DefaultLocationGenerator { let table_location = table_metadata.location(); let prop = table_metadata.properties(); let configured_data_location = prop - .get(WRITE_DATA_LOCATION) - .or(prop.get(WRITE_FOLDER_STORAGE_LOCATION)); + .get(TableProperties::PROPERTY_WRITE_DATA_LOCATION) + .or(prop.get(TableProperties::PROPERTY_WRITE_FOLDER_STORAGE_LOCATION)); let data_location = if let Some(data_location) = configured_data_location { data_location.clone() } else { - format!("{table_location}{DEFAULT_DATA_DIR}") + format!("{table_location}{}", TableProperties::DEFAULT_DATA_DIR) }; Ok(Self { data_location }) } @@ -144,9 +134,9 @@ impl ObjectStorageLocationGenerator { }; let include_partition_paths = prop - .get(WRITE_OBJECT_STORAGE_PARTITIONED_PATHS) + .get(TableProperties::PROPERTY_WRITE_OBJECT_STORAGE_PARTITIONED_PATHS) .and_then(|value| value.parse::().ok()) - .unwrap_or(WRITE_OBJECT_STORAGE_PARTITIONED_PATHS_DEFAULT); + .unwrap_or(TableProperties::WRITE_OBJECT_STORAGE_PARTITIONED_PATHS_DEFAULT); Ok(Self { storage_location, @@ -246,11 +236,11 @@ fn resolve_data_location( table_location: &str, ) -> String { properties - .get(WRITE_DATA_LOCATION) - .or_else(|| properties.get(WRITE_OBJECT_STORAGE_LOCATION)) - .or_else(|| properties.get(WRITE_FOLDER_STORAGE_LOCATION)) + .get(TableProperties::PROPERTY_WRITE_DATA_LOCATION) + .or_else(|| properties.get(TableProperties::PROPERTY_WRITE_OBJECT_STORAGE_LOCATION)) + .or_else(|| properties.get(TableProperties::PROPERTY_WRITE_FOLDER_STORAGE_LOCATION)) .cloned() - .unwrap_or_else(|| format!("{table_location}{DEFAULT_DATA_DIR}")) + .unwrap_or_else(|| format!("{table_location}{}", TableProperties::DEFAULT_DATA_DIR)) } /// `FileNameGeneratorTrait` used to generate file name for data file. The file name can be passed to `LocationGenerator` to generate the location of the file. @@ -310,11 +300,10 @@ pub(crate) mod test { use super::LocationGenerator; use crate::spec::{ FormatVersion, Literal, NestedField, PartitionKey, PartitionSpec, PrimitiveType, Schema, - Struct, StructType, TableMetadata, Transform, Type, + Struct, StructType, TableMetadata, TableProperties, Transform, Type, }; use crate::writer::file_writer::location_generator::{ DefaultLocationGenerator, FileNameGenerator, ObjectStorageLocationGenerator, - WRITE_DATA_LOCATION, WRITE_FOLDER_STORAGE_LOCATION, WRITE_OBJECT_STORAGE_PARTITIONED_PATHS, }; /// Build a minimal `TableMetadata` for location generator tests. @@ -390,7 +379,7 @@ pub(crate) mod test { // test custom data location table_metadata.properties.insert( - WRITE_FOLDER_STORAGE_LOCATION.to_string(), + TableProperties::PROPERTY_WRITE_FOLDER_STORAGE_LOCATION.to_string(), "s3://data.db/table/data_1".to_string(), ); let location_generator = DefaultLocationGenerator::new(&table_metadata).unwrap(); @@ -402,7 +391,7 @@ pub(crate) mod test { ); table_metadata.properties.insert( - WRITE_DATA_LOCATION.to_string(), + TableProperties::PROPERTY_WRITE_DATA_LOCATION.to_string(), "s3://data.db/table/data_2".to_string(), ); let location_generator = DefaultLocationGenerator::new(&table_metadata).unwrap(); @@ -414,7 +403,7 @@ pub(crate) mod test { ); table_metadata.properties.insert( - WRITE_DATA_LOCATION.to_string(), + TableProperties::PROPERTY_WRITE_DATA_LOCATION.to_string(), // invalid table location "s3://data.db/data_3".to_string(), ); @@ -610,7 +599,7 @@ pub(crate) mod test { let table_metadata = table_metadata_with( "s3://data.db/table", HashMap::from([( - WRITE_OBJECT_STORAGE_PARTITIONED_PATHS.to_string(), + TableProperties::PROPERTY_WRITE_OBJECT_STORAGE_PARTITIONED_PATHS.to_string(), "true".to_string(), )]), ); @@ -648,7 +637,7 @@ pub(crate) mod test { let table_metadata = table_metadata_with( "s3://data.db/table", HashMap::from([( - WRITE_OBJECT_STORAGE_PARTITIONED_PATHS.to_string(), + TableProperties::PROPERTY_WRITE_OBJECT_STORAGE_PARTITIONED_PATHS.to_string(), "false".to_string(), )]), ); @@ -668,7 +657,7 @@ pub(crate) mod test { let table_metadata = table_metadata_with( "s3://data.db/table", HashMap::from([( - WRITE_DATA_LOCATION.to_string(), + TableProperties::PROPERTY_WRITE_DATA_LOCATION.to_string(), "s3://custom-bucket/objects".to_string(), )]), ); @@ -689,11 +678,11 @@ pub(crate) mod test { "s3://data.db/table", HashMap::from([ ( - WRITE_DATA_LOCATION.to_string(), + TableProperties::PROPERTY_WRITE_DATA_LOCATION.to_string(), "s3://data.db/table/data_primary".to_string(), ), ( - WRITE_FOLDER_STORAGE_LOCATION.to_string(), + TableProperties::PROPERTY_WRITE_FOLDER_STORAGE_LOCATION.to_string(), "s3://data.db/table/data_legacy".to_string(), ), ]), From e08062225a851482647cbdaabb255ecc7eb7d090 Mon Sep 17 00:00:00 2001 From: huanghsiang_cheng Date: Thu, 30 Jul 2026 17:00:50 -0700 Subject: [PATCH 06/11] Reuse strip_trailing_slash --- crates/iceberg/src/spec/table_properties.rs | 2 +- .../iceberg/src/writer/file_writer/location_generator.rs | 9 +++------ 2 files changed, 4 insertions(+), 7 deletions(-) diff --git a/crates/iceberg/src/spec/table_properties.rs b/crates/iceberg/src/spec/table_properties.rs index 5bc209b7b3..310732ba3d 100644 --- a/crates/iceberg/src/spec/table_properties.rs +++ b/crates/iceberg/src/spec/table_properties.rs @@ -41,7 +41,7 @@ where } /// Strips trailing slashes from a location, preserving a bare URI scheme root -fn strip_trailing_slash(path: &str) -> &str { +pub(crate) fn strip_trailing_slash(path: &str) -> &str { let mut path = path; while !path.ends_with("://") { let Some(stripped) = path.strip_suffix('/') else { diff --git a/crates/iceberg/src/writer/file_writer/location_generator.rs b/crates/iceberg/src/writer/file_writer/location_generator.rs index 01295d4ab5..107b37da36 100644 --- a/crates/iceberg/src/writer/file_writer/location_generator.rs +++ b/crates/iceberg/src/writer/file_writer/location_generator.rs @@ -21,7 +21,9 @@ use std::sync::Arc; use std::sync::atomic::AtomicU64; use crate::Result; -use crate::spec::{DataFileFormat, PartitionKey, TableMetadata, TableProperties}; +use crate::spec::{ + DataFileFormat, PartitionKey, TableMetadata, TableProperties, strip_trailing_slash, +}; /// `LocationGenerator` used to generate the location of data file. pub trait LocationGenerator: Clone + Send + Sync + 'static { @@ -185,11 +187,6 @@ impl LocationGenerator for ObjectStorageLocationGenerator { } } -/// Strip a single trailing slash from a location, if present. -fn strip_trailing_slash(location: &str) -> &str { - location.strip_suffix('/').unwrap_or(location) -} - /// Derive the `{parent}/{name}` context from a table location, mirroring Hadoop's /// `Path.getParent().getName()` / `Path.getName()`. fn path_context(table_location: &str) -> String { From ad243b56e30dfcc1b85ad7eec4bb2cc4b03a382a Mon Sep 17 00:00:00 2001 From: huanghsiang_cheng Date: Thu, 30 Jul 2026 17:32:06 -0700 Subject: [PATCH 07/11] Remove redundant padding logic --- crates/iceberg/src/writer/file_writer/location_generator.rs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/crates/iceberg/src/writer/file_writer/location_generator.rs b/crates/iceberg/src/writer/file_writer/location_generator.rs index 107b37da36..ca5bc4b997 100644 --- a/crates/iceberg/src/writer/file_writer/location_generator.rs +++ b/crates/iceberg/src/writer/file_writer/location_generator.rs @@ -166,10 +166,7 @@ impl ObjectStorageLocationGenerator { 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. - let binary = format!("{:032b}", hash_code | 0x8000_0000); + let binary = format!("{:032b}", hash_code); let hash = &binary[binary.len() - HASH_BINARY_STRING_BITS..]; dirs_from_hash(hash) } From 0522dcec88400dd77b0335a90a6bc172f6a44421 Mon Sep 17 00:00:00 2001 From: huanghsiang_cheng Date: Thu, 30 Jul 2026 17:47:04 -0700 Subject: [PATCH 08/11] Reenable the test --- .../writer/file_writer/location_generator.rs | 91 ++++++------------- 1 file changed, 30 insertions(+), 61 deletions(-) diff --git a/crates/iceberg/src/writer/file_writer/location_generator.rs b/crates/iceberg/src/writer/file_writer/location_generator.rs index ca5bc4b997..4328c0efcb 100644 --- a/crates/iceberg/src/writer/file_writer/location_generator.rs +++ b/crates/iceberg/src/writer/file_writer/location_generator.rs @@ -300,36 +300,6 @@ pub(crate) mod test { DefaultLocationGenerator, FileNameGenerator, ObjectStorageLocationGenerator, }; - /// Build a minimal `TableMetadata` for location generator tests. - fn table_metadata_with(location: &str, properties: HashMap) -> TableMetadata { - TableMetadata { - format_version: FormatVersion::V2, - table_uuid: Uuid::parse_str("fb072c92-a02b-11e9-ae9c-1bb7bc9eca94").unwrap(), - location: location.to_string(), - last_updated_ms: 1515100955770, - last_column_id: 1, - schemas: HashMap::new(), - current_schema_id: 1, - partition_specs: HashMap::new(), - default_spec: PartitionSpec::unpartition_spec().into(), - default_partition_type: StructType::new(vec![]), - last_partition_id: 1000, - default_sort_order_id: 0, - sort_orders: HashMap::from_iter(vec![]), - snapshots: HashMap::default(), - current_snapshot_id: None, - last_sequence_number: 1, - properties, - snapshot_log: Vec::new(), - metadata_log: vec![], - refs: HashMap::new(), - statistics: HashMap::new(), - partition_statistics: HashMap::new(), - encryption_keys: HashMap::new(), - next_row_id: 0, - } - } - #[test] fn test_default_location_generate() { let mut table_metadata = TableMetadata { @@ -483,36 +453,6 @@ pub(crate) mod test { ); } - /// Build a minimal `TableMetadata` for location generator tests. - fn table_metadata_with(location: &str, properties: HashMap) -> TableMetadata { - TableMetadata { - format_version: FormatVersion::V2, - table_uuid: Uuid::parse_str("fb072c92-a02b-11e9-ae9c-1bb7bc9eca94").unwrap(), - location: location.to_string(), - last_updated_ms: 1515100955770, - last_column_id: 2, - schemas: HashMap::new(), - current_schema_id: 1, - partition_specs: HashMap::new(), - default_spec: PartitionSpec::unpartition_spec().into(), - default_partition_type: StructType::new(vec![]), - last_partition_id: 1000, - default_sort_order_id: 0, - sort_orders: HashMap::from_iter(vec![]), - snapshots: HashMap::default(), - current_snapshot_id: None, - last_sequence_number: 1, - properties, - snapshot_log: Vec::new(), - metadata_log: vec![], - refs: HashMap::new(), - statistics: HashMap::new(), - partition_statistics: HashMap::new(), - encryption_keys: HashMap::new(), - next_row_id: 0, - } - } - #[test] fn test_object_storage_hash_injection() { // Golden vectors ported from Java's TestLocationProvider#testHashInjection, verifying that @@ -570,7 +510,6 @@ pub(crate) mod test { ); } - #[ignore] #[test] fn test_object_storage_include_partition_paths_with_special_characters() { let schema = Arc::new( @@ -689,4 +628,34 @@ pub(crate) mod test { "s3://data.db/table/data_primary/0101/0110/1001/10110010/a" ); } + + /// Build a minimal `TableMetadata` for location generator tests. + fn table_metadata_with(location: &str, properties: HashMap) -> TableMetadata { + TableMetadata { + format_version: FormatVersion::V2, + table_uuid: Uuid::parse_str("fb072c92-a02b-11e9-ae9c-1bb7bc9eca94").unwrap(), + location: location.to_string(), + last_updated_ms: 1515100955770, + last_column_id: 2, + schemas: HashMap::new(), + current_schema_id: 1, + partition_specs: HashMap::new(), + default_spec: PartitionSpec::unpartition_spec().into(), + default_partition_type: StructType::new(vec![]), + last_partition_id: 1000, + default_sort_order_id: 0, + sort_orders: HashMap::from_iter(vec![]), + snapshots: HashMap::default(), + current_snapshot_id: None, + last_sequence_number: 1, + properties, + snapshot_log: Vec::new(), + metadata_log: vec![], + refs: HashMap::new(), + statistics: HashMap::new(), + partition_statistics: HashMap::new(), + encryption_keys: HashMap::new(), + next_row_id: 0, + } + } } From 1490fb755d3dde4c0547106ab019069dc6fcb43c Mon Sep 17 00:00:00 2001 From: huanghsiang_cheng Date: Thu, 30 Jul 2026 18:12:13 -0700 Subject: [PATCH 09/11] Remove DEFAULT_DATA_DIR from public API; rename a property --- crates/iceberg/public-api.txt | 9 +++++++++ crates/iceberg/src/spec/table_properties.rs | 4 +--- .../iceberg/src/writer/file_writer/location_generator.rs | 9 ++++++--- 3 files changed, 16 insertions(+), 6 deletions(-) diff --git a/crates/iceberg/public-api.txt b/crates/iceberg/public-api.txt index 73a453c594..f9d3eee310 100644 --- a/crates/iceberg/public-api.txt +++ b/crates/iceberg/public-api.txt @@ -2762,9 +2762,13 @@ pub iceberg::spec::TableProperties::max_ref_age_ms: i64 pub iceberg::spec::TableProperties::max_snapshot_age_ms: i64 pub iceberg::spec::TableProperties::metadata_compression_codec: iceberg::compression::CompressionCodec pub iceberg::spec::TableProperties::min_snapshots_to_keep: usize +pub iceberg::spec::TableProperties::write_data_location: core::option::Option pub iceberg::spec::TableProperties::write_datafusion_fanout_enabled: bool +pub iceberg::spec::TableProperties::write_folder_storage_location: core::option::Option pub iceberg::spec::TableProperties::write_format_default: alloc::string::String pub iceberg::spec::TableProperties::write_metadata_path: core::option::Option +pub iceberg::spec::TableProperties::write_object_storage_location: core::option::Option +pub iceberg::spec::TableProperties::write_object_storage_partitioned_paths: core::option::Option pub iceberg::spec::TableProperties::write_target_file_size_bytes: usize impl iceberg::spec::TableProperties pub const iceberg::spec::TableProperties::PROPERTY_COMMIT_MAX_RETRY_WAIT_MS: &str @@ -2812,7 +2816,12 @@ pub const iceberg::spec::TableProperties::PROPERTY_PARQUET_CDC_NORM_LEVEL: &str pub const iceberg::spec::TableProperties::PROPERTY_PARQUET_CDC_NORM_LEVEL_DEFAULT: i32 pub const iceberg::spec::TableProperties::PROPERTY_SNAPSHOT_COUNT: &str pub const iceberg::spec::TableProperties::PROPERTY_UUID: &str +pub const iceberg::spec::TableProperties::PROPERTY_WRITE_DATA_LOCATION: &str +pub const iceberg::spec::TableProperties::PROPERTY_WRITE_FOLDER_STORAGE_LOCATION: &str pub const iceberg::spec::TableProperties::PROPERTY_WRITE_METADATA_PATH: &str +pub const iceberg::spec::TableProperties::PROPERTY_WRITE_OBJECT_STORAGE_LOCATION: &str +pub const iceberg::spec::TableProperties::PROPERTY_WRITE_OBJECT_STORAGE_PARTITIONED_PATHS: &str +pub const iceberg::spec::TableProperties::PROPERTY_WRITE_OBJECT_STORAGE_PARTITIONED_PATHS_DEFAULT: bool pub const iceberg::spec::TableProperties::PROPERTY_WRITE_PARTITION_SUMMARY_LIMIT: &str pub const iceberg::spec::TableProperties::PROPERTY_WRITE_PARTITION_SUMMARY_LIMIT_DEFAULT: u64 pub const iceberg::spec::TableProperties::PROPERTY_WRITE_TARGET_FILE_SIZE_BYTES: &str diff --git a/crates/iceberg/src/spec/table_properties.rs b/crates/iceberg/src/spec/table_properties.rs index 310732ba3d..9e1026286e 100644 --- a/crates/iceberg/src/spec/table_properties.rs +++ b/crates/iceberg/src/spec/table_properties.rs @@ -337,15 +337,13 @@ impl TableProperties { pub const PROPERTY_WRITE_DATA_LOCATION: &str = "write.data.path"; /// Property key for deprecated [write_folder_storage_location] pub const PROPERTY_WRITE_FOLDER_STORAGE_LOCATION: &str = "write.folder-storage.path"; - /// Default directory for data files - pub const DEFAULT_DATA_DIR: &str = "/data"; /// Property key for deprecated object storage path, kept as a fallback for compatibility. pub const PROPERTY_WRITE_OBJECT_STORAGE_LOCATION: &str = "write.object-storage.path"; /// Property key for controlling whether partition values are included in object storage paths. pub const PROPERTY_WRITE_OBJECT_STORAGE_PARTITIONED_PATHS: &str = "write.object-storage.partitioned-paths"; /// Default value for [PROPERTY_WRITE_OBJECT_STORAGE_PARTITIONED_PATHS] - pub const WRITE_OBJECT_STORAGE_PARTITIONED_PATHS_DEFAULT: bool = true; + pub const PROPERTY_WRITE_OBJECT_STORAGE_PARTITIONED_PATHS_DEFAULT: bool = true; } impl TryFrom<&HashMap> for TableProperties { diff --git a/crates/iceberg/src/writer/file_writer/location_generator.rs b/crates/iceberg/src/writer/file_writer/location_generator.rs index 4328c0efcb..e645525de4 100644 --- a/crates/iceberg/src/writer/file_writer/location_generator.rs +++ b/crates/iceberg/src/writer/file_writer/location_generator.rs @@ -50,6 +50,9 @@ const ENTROPY_DIR_LENGTH: usize = 4; /// Number of entropy directories generated from the hash. const ENTROPY_DIR_DEPTH: usize = 3; +/// Default directory for data files +const DEFAULT_DATA_DIR: &str = "/data"; + #[derive(Clone, Debug)] /// `DefaultLocationGenerator` used to generate the data dir location of data file. /// The location is generated based on the table location and the data location in table properties. @@ -68,7 +71,7 @@ impl DefaultLocationGenerator { let data_location = if let Some(data_location) = configured_data_location { data_location.clone() } else { - format!("{table_location}{}", TableProperties::DEFAULT_DATA_DIR) + format!("{table_location}{DEFAULT_DATA_DIR}") }; Ok(Self { data_location }) } @@ -138,7 +141,7 @@ impl ObjectStorageLocationGenerator { let include_partition_paths = prop .get(TableProperties::PROPERTY_WRITE_OBJECT_STORAGE_PARTITIONED_PATHS) .and_then(|value| value.parse::().ok()) - .unwrap_or(TableProperties::WRITE_OBJECT_STORAGE_PARTITIONED_PATHS_DEFAULT); + .unwrap_or(TableProperties::PROPERTY_WRITE_OBJECT_STORAGE_PARTITIONED_PATHS_DEFAULT); Ok(Self { storage_location, @@ -234,7 +237,7 @@ fn resolve_data_location( .or_else(|| properties.get(TableProperties::PROPERTY_WRITE_OBJECT_STORAGE_LOCATION)) .or_else(|| properties.get(TableProperties::PROPERTY_WRITE_FOLDER_STORAGE_LOCATION)) .cloned() - .unwrap_or_else(|| format!("{table_location}{}", TableProperties::DEFAULT_DATA_DIR)) + .unwrap_or_else(|| format!("{table_location}{DEFAULT_DATA_DIR}")) } /// `FileNameGeneratorTrait` used to generate file name for data file. The file name can be passed to `LocationGenerator` to generate the location of the file. From 0e66794fe889fa5e28d53b67a51ed51bfb3b0357 Mon Sep 17 00:00:00 2001 From: huanghsiang_cheng Date: Fri, 31 Jul 2026 09:49:28 -0700 Subject: [PATCH 10/11] Move strip_trailing_slash to util module --- crates/iceberg/src/spec/table_properties.rs | 16 ++--------- crates/iceberg/src/util/location.rs | 28 +++++++++++++++++++ crates/iceberg/src/util/mod.rs | 1 + .../writer/file_writer/location_generator.rs | 10 +++---- 4 files changed, 36 insertions(+), 19 deletions(-) create mode 100644 crates/iceberg/src/util/location.rs diff --git a/crates/iceberg/src/spec/table_properties.rs b/crates/iceberg/src/spec/table_properties.rs index 9e1026286e..a862e9adee 100644 --- a/crates/iceberg/src/spec/table_properties.rs +++ b/crates/iceberg/src/spec/table_properties.rs @@ -21,6 +21,7 @@ use std::str::FromStr; use crate::compression::CompressionCodec; use crate::error::{Error, ErrorKind, Result}; +use crate::util::location; fn parse_property( properties: &HashMap, @@ -40,18 +41,6 @@ where }) } -/// Strips trailing slashes from a location, preserving a bare URI scheme root -pub(crate) fn strip_trailing_slash(path: &str) -> &str { - let mut path = path; - while !path.ends_with("://") { - let Some(stripped) = path.strip_suffix('/') else { - break; - }; - path = stripped; - } - path -} - fn parse_location_property( properties: &HashMap, key: &str, @@ -66,7 +55,7 @@ fn parse_location_property( )); } - Ok(strip_trailing_slash(path).to_string()) + Ok(location::strip_trailing_slash(path).to_string()) }) .transpose() } @@ -460,6 +449,7 @@ impl TryFrom<&HashMap> for TableProperties { mod tests { use super::*; use crate::compression::CompressionCodec; + use crate::util::location::strip_trailing_slash; #[test] fn test_table_properties_default() { diff --git a/crates/iceberg/src/util/location.rs b/crates/iceberg/src/util/location.rs new file mode 100644 index 0000000000..a793593ce6 --- /dev/null +++ b/crates/iceberg/src/util/location.rs @@ -0,0 +1,28 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +/// Strips trailing slashes from a location, preserving a bare URI scheme root +pub(crate) fn strip_trailing_slash(path: &str) -> &str { + let mut path = path; + while !path.ends_with("://") { + let Some(stripped) = path.strip_suffix('/') else { + break; + }; + path = stripped; + } + path +} diff --git a/crates/iceberg/src/util/mod.rs b/crates/iceberg/src/util/mod.rs index 3cf2eef9b0..79ed47a57a 100644 --- a/crates/iceberg/src/util/mod.rs +++ b/crates/iceberg/src/util/mod.rs @@ -17,6 +17,7 @@ use std::num::NonZeroUsize; +pub(crate) mod location; /// Utilities for working with snapshots. pub mod snapshot; diff --git a/crates/iceberg/src/writer/file_writer/location_generator.rs b/crates/iceberg/src/writer/file_writer/location_generator.rs index e645525de4..998b6e8dcf 100644 --- a/crates/iceberg/src/writer/file_writer/location_generator.rs +++ b/crates/iceberg/src/writer/file_writer/location_generator.rs @@ -21,9 +21,8 @@ use std::sync::Arc; use std::sync::atomic::AtomicU64; use crate::Result; -use crate::spec::{ - DataFileFormat, PartitionKey, TableMetadata, TableProperties, strip_trailing_slash, -}; +use crate::spec::{DataFileFormat, PartitionKey, TableMetadata, TableProperties}; +use crate::util::location::strip_trailing_slash; /// `LocationGenerator` used to generate the location of data file. pub trait LocationGenerator: Clone + Send + Sync + 'static { @@ -43,6 +42,8 @@ pub trait LocationGenerator: Clone + Send + Sync + 'static { fn generate_location(&self, partition_key: Option<&PartitionKey>, file_name: &str) -> String; } +/// Default directory for data files +const DEFAULT_DATA_DIR: &str = "/data"; /// Number of trailing hash bits used to build the entropy directories. const HASH_BINARY_STRING_BITS: usize = 20; /// Length of each entropy directory. @@ -50,9 +51,6 @@ const ENTROPY_DIR_LENGTH: usize = 4; /// Number of entropy directories generated from the hash. const ENTROPY_DIR_DEPTH: usize = 3; -/// Default directory for data files -const DEFAULT_DATA_DIR: &str = "/data"; - #[derive(Clone, Debug)] /// `DefaultLocationGenerator` used to generate the data dir location of data file. /// The location is generated based on the table location and the data location in table properties. From 00644b48b83d25636fc9a5713896ba4c221ec82c Mon Sep 17 00:00:00 2001 From: huanghsiang_cheng Date: Fri, 31 Jul 2026 09:52:19 -0700 Subject: [PATCH 11/11] Clean up --- crates/iceberg/src/spec/table_properties.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/crates/iceberg/src/spec/table_properties.rs b/crates/iceberg/src/spec/table_properties.rs index a862e9adee..db432db09c 100644 --- a/crates/iceberg/src/spec/table_properties.rs +++ b/crates/iceberg/src/spec/table_properties.rs @@ -21,7 +21,7 @@ use std::str::FromStr; use crate::compression::CompressionCodec; use crate::error::{Error, ErrorKind, Result}; -use crate::util::location; +use crate::util::location::strip_trailing_slash; fn parse_property( properties: &HashMap, @@ -55,7 +55,7 @@ fn parse_location_property( )); } - Ok(location::strip_trailing_slash(path).to_string()) + Ok(strip_trailing_slash(path).to_string()) }) .transpose() } @@ -449,7 +449,6 @@ impl TryFrom<&HashMap> for TableProperties { mod tests { use super::*; use crate::compression::CompressionCodec; - use crate::util::location::strip_trailing_slash; #[test] fn test_table_properties_default() {