Url encode field names for partition paths (apache#10329)

* Url encode field ids for partition paths * Cleanup tests * Add test for partition paths
manuzhang · May 27, 2024 · 795fea9 · 795fea9
1 parent 580df62
commit 795fea9
Show file tree

Hide file tree

Showing 3 changed files with 31 additions and 2 deletions.
diff --git a/api/src/main/java/org/apache/iceberg/PartitionSpec.java b/api/src/main/java/org/apache/iceberg/PartitionSpec.java
@@ -189,7 +189,7 @@ public String partitionToPath(StructLike data) {
       if (i > 0) {
         sb.append("/");
       }
-      sb.append(field.name()).append("=").append(escape(valueString));
+      sb.append(escape(field.name())).append("=").append(escape(valueString));
     }
     return sb.toString();
   }

diff --git a/api/src/test/java/org/apache/iceberg/TestPartitionPaths.java b/api/src/test/java/org/apache/iceberg/TestPartitionPaths.java
@@ -32,7 +32,8 @@ public class TestPartitionPaths {
       new Schema(
           Types.NestedField.required(1, "id", Types.IntegerType.get()),
           Types.NestedField.optional(2, "data", Types.StringType.get()),
-          Types.NestedField.optional(3, "ts", Types.TimestampType.withoutZone()));
+          Types.NestedField.optional(3, "ts", Types.TimestampType.withoutZone()),
+          Types.NestedField.optional(4, "\"esc\"#1", Types.StringType.get()));
 
   @Test
   public void testPartitionPath() {
@@ -62,4 +63,13 @@ public void testEscapedStrings() {
         .as("Should escape / as %2F")
         .isEqualTo("data=a%2Fb%2Fc%2Fd/data_trunc=a%2Fb%2Fc%2Fd");
   }
+
+  @Test
+  public void testEscapedFieldNames() {
+    PartitionSpec spec = PartitionSpec.builderFor(SCHEMA).identity("\"esc\"#1").build();
+
+    assertThat(spec.partitionToPath(Row.of("a/b/c/d")))
+        .as("Should escape \" as %22 and # as %23")
+        .isEqualTo("%22esc%22%231=a%2Fb%2Fc%2Fd");
+  }
 }
diff --git a/core/src/test/java/org/apache/iceberg/TestLocationProvider.java b/core/src/test/java/org/apache/iceberg/TestLocationProvider.java
@@ -26,6 +26,7 @@
 import java.util.Map;
 import org.apache.iceberg.io.LocationProvider;
 import org.apache.iceberg.relocated.com.google.common.base.Splitter;
+import org.apache.iceberg.types.Types;
 import org.junit.jupiter.api.TestTemplate;
 import org.junit.jupiter.api.extension.ExtendWith;
 
@@ -285,4 +286,22 @@ public void testObjectStorageWithinTableLocation() {
     assertThat(parts).element(2).asString().isNotEmpty();
     assertThat(parts).element(3).asString().isEqualTo("test.parquet");
   }
+
+  @TestTemplate
+  public void testEncodedFieldNameInPartitionPath() {
+    // Update the table to use a string field for partitioning with special characters in the name
+    table.updateProperties().set(TableProperties.OBJECT_STORE_ENABLED, "true").commit();
+    table.updateSchema().addColumn("data#1", Types.StringType.get()).commit();
+    table.updateSpec().addField("data#1").commit();
+
+    // Use a partition value that has a special character
+    StructLike partitionData = TestHelpers.CustomRow.of(0, "val#1");
+
+    String fileLocation =
+        table.locationProvider().newDataLocation(table.spec(), partitionData, "test.parquet");
+    List<String> parts = Splitter.on("/").splitToList(fileLocation);
+    String partitionString = parts.get(parts.size() - 2);
+
+    assertThat(partitionString).isEqualTo("data%231=val%231");
+  }
 }