andygrove commented on code in PR #5651:
URL: https://github.com/apache/datafusion-comet/pull/5651#discussion_r3928474181
##########
spark/src/test/scala/org/apache/comet/CometIcebergWriteActionSuite.scala:
##########
@@ -1172,6 +1172,59 @@ class CometIcebergWriteActionSuite
}
}
+ // iceberg-java's `PartitionSpec.partitionToPath` runs every partition name
and value through
+ // `URLEncoder.encode`, and iceberg-rust's `partition_to_path` uses the same
+ // application/x-www-form-urlencoded table (apache/iceberg-rust#2875).
Readers resolve files
+ // through manifest metadata rather than paths, but the committed location
still has to be one
+ // every FileIO can open: an unescaped `#` in an S3 key is a fragment
delimiter to `S3FileIO`.
+ test("native acceleration: partition paths are URL-escaped like
iceberg-java") {
+ assumeNativeAcceleration()
+ withIcebergCatalog { warehouseDir =>
+ createTable(warehouseDir, "escaped_native", partitionSpec = "PARTITIONED
BY (region)")
+ createTable(warehouseDir, "escaped_jvm", partitionSpec = "PARTITIONED BY
(region)")
+ val regions = Seq("a/b", "c#d", "e?f", "g h", "i=j", "k%l", "m+n",
"*-._", "日本", "")
+ val values = regions.zipWithIndex
+ .map { case (region, i) => s"($i, '$region', $i.5)" }
+ .mkString(", ")
+
+ spark.sql(s"INSERT INTO $catalog.$ns.escaped_native VALUES $values")
Review Comment:
You were right, and the test was not exercising the native writer.
`spark.comet.iceberg.write.enabled` is off by default, so both inserts went
through the JVM writer and the comparison was JVM vs JVM.
Fixed in e0bd7bfe9: the native insert is now wrapped in
`assertNativeWriteEngages("escaped_native", regions.indices)`, which flips the
conf, enables the local-table-scan conversion the inline `VALUES` input needs,
and asserts a `CometIcebergWriteExec` in the captured plan. The JVM twin now
runs on the suite defaults instead of toggling `splitOperator.enabled`,
matching the other parity tests.
To confirm the repaired test is genuine regression coverage, I rebuilt the
native library against both pins.
Old pin (`3d84c81`), fails:
```
- native acceleration: partition paths are URL-escaped like iceberg-java ***
FAILED ***
HashSet("region=e?f", "region=g h", "region=i=j", "region=",
"region=*-._", "region=k%l",
"region=a/b", "region=c#d", "region=日本", "region=m+n")
did not equal
HashSet("region=a%2Fb", "region=e%3Ff", "region=",
"region=%E6%97%A5%E6%9C%AC", "region=*-._",
"region=k%25l", "region=g+h", "region=i%3Dj", "region=c%23d",
"region=m%2Bn")
(CometIcebergWriteActionSuite.scala:1210)
Tests: succeeded 0, failed 1, canceled 0, ignored 0, pending 0
```
New pin (`8adaa87`), passes:
```
- native acceleration: partition paths are URL-escaped like iceberg-java (8
seconds, 956 milliseconds)
Tests: succeeded 1, failed 0, canceled 0, ignored 0, pending 0
```
The old-pin output is the failure mode this PR is about: `region=a/b` splits
into a nested directory, and `region=c#d` lands raw, which is the key
`S3FileIO` would resolve differently. The full `CometIcebergWriteActionSuite`
on the new pin is 53 succeeded, 0 failed.
--
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]
