dannycjones commented on code in PR #2871:
URL: https://github.com/apache/iceberg-rust/pull/2871#discussion_r3698811372
##########
crates/iceberg/src/spec/table_properties.rs:
##########
Review Comment:
I agree, we should add tests for reading the properties in this way too.
i.e. a test asserting that `write.object-storage.partitioned-paths=False`
will correctly map to `false`, and
`write.object-storage.partitioned-paths=True` to `true`
##########
crates/iceberg/src/writer/file_writer/location_generator.rs:
##########
@@ -310,6 +454,182 @@ pub(crate) mod test {
);
}
+ #[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);
+ }
+ }
Review Comment:
Nice, matches this:
https://github.com/apache/iceberg/blob/3c356c45ea00b6ac036684f8b7e24a326c2a55e8/core/src/test/java/org/apache/iceberg/TestLocationProvider.java#L302-L313
nitpick: I think it would be good to include a comment with permalink like
this when referring to sources for ported code.
##########
crates/iceberg/src/util/mod.rs:
##########
@@ -17,6 +17,7 @@
use std::num::NonZeroUsize;
+pub(crate) mod location;
Review Comment:
Please also add a short Rustdoc, similar to other entries in this module.
##########
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>,
Review Comment:
The deprecation path for these properties is super unclear. Iceberg Java
suggests it will be removed in iceberg-java v2.
I'd like to be more explicit that the removal date isn't specified right now.
I suspect that the deprecation of table properties isn't well defined right
now and is subject to change.
```suggestion
/// Deprecated table property for data file write location.
///
/// Property will be removed at a later date.
/// Superseded by [write_data_location].
pub write_folder_storage_location: Option<String>,
/// Deprecated table property for data file write location for object
storage location generator.
///
/// Property will be removed at a later date.
/// Superseded by [write_data_location].
pub write_object_storage_location: Option<String>,
```
--
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]
