CTTY commented on code in PR #2871:
URL: https://github.com/apache/iceberg-rust/pull/2871#discussion_r3693891936
##########
crates/iceberg/src/util/mod.rs:
##########
@@ -17,6 +17,7 @@
use std::num::NonZeroUsize;
+pub(crate) mod location;
Review Comment:
I think this can be pub?
##########
crates/iceberg/src/writer/file_writer/location_generator.rs:
##########
@@ -93,6 +99,145 @@ 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(TableProperties::PROPERTY_WRITE_OBJECT_STORAGE_PARTITIONED_PATHS)
+ .and_then(|value| value.parse::<bool>().ok())
+
.unwrap_or(TableProperties::PROPERTY_WRITE_OBJECT_STORAGE_PARTITIONED_PATHS_DEFAULT);
Review Comment:
We should not parse the value here, let's use tableProperties::parse_property
##########
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 {
Review Comment:
There is a unit test in table_properties.rs for this function, could you
move that here as well?
##########
crates/iceberg/src/spec/table_properties.rs:
##########
Review 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_bool_property` 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
##########
crates/iceberg/src/spec/table_properties.rs:
##########
@@ -173,6 +162,14 @@ pub struct TableProperties {
pub encryption_key_id: Option<String>,
/// The encryption data encryption key length in bytes.
pub encryption_data_key_length: usize,
+ /// Base directory for data files
+ pub write_data_location: Option<String>,
+ /// Deprecated and will be removed in Iceberg Java, use
[write_data_location] instead.
+ pub write_folder_storage_location: Option<String>,
+ /// 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>,
Review Comment:
Should this be `Option<bool>`?
--
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]
