CTTY commented on code in PR #2875:
URL: https://github.com/apache/iceberg-rust/pull/2875#discussion_r3641621444
##########
crates/iceberg/Cargo.toml:
##########
@@ -79,6 +79,7 @@ tracing = { workspace = true }
typed-builder = { workspace = true }
typetag = { workspace = true }
url = { workspace = true }
+urlencoding = "2.1.3"
Review Comment:
We should maintain this version in the root Cargo.toml
##########
crates/iceberg/src/spec/partition.rs:
##########
@@ -168,10 +168,14 @@ impl PartitionSpec {
let value = data[i].as_ref();
format!(
"{}={}",
- field.name,
- field
- .transform
- .to_human_string(&field_types[i].field_type, value)
+ urlencoding::encode(field.name.as_str()).into_owned(),
Review Comment:
I took a look at Java's code and noticed that java's `URLEncoder` actually
follows `application/x-www-form-urlencoded` rather than RFC 3986. If we want to
be as close as to java's behavior, then we should use `form_urlencoded`, which
implements the `application/x-www-form-urlencoded` serializer
This actually makes a difference as `form_urlencoded` and java `URLEncoder`
will convert space to `+`, while `urlencoding` will convert space to `%20`. and
space is the most common special character used
##########
crates/iceberg/src/writer/file_writer/location_generator.rs:
##########
@@ -270,10 +270,52 @@ pub(crate) mod test {
assert_eq!(location, "/base/path/id=42/name=alice/data-00000.parquet");
// Create a table metadata for DefaultLocationGenerator
- let table_metadata = TableMetadata {
+ let table_metadata = table_metadata_with("s3://data.db/table",
HashMap::new());
+
+ // Test with DefaultLocationGenerator
+ let default_location_gen =
DefaultLocationGenerator::new(&table_metadata).unwrap();
+ let location =
default_location_gen.generate_location(Some(&partition_key), file_name);
+ assert_eq!(
+ location,
+ "s3://data.db/table/data/id=42/name=alice/data-00000.parquet"
+ );
+ }
+
+ #[test]
+ fn test_location_generate_with_special_characters_partitionq() {
Review Comment:
typo:
```suggestion
fn test_location_generate_with_special_characters_partition() {
```
--
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]
