andygrove commented on PR #5763:
URL:
https://github.com/apache/datafusion-comet/pull/5763#issuecomment-5608023654
You were right, and the reason the first guard missed it is worth writing
down: `percent_encoding`'s `should_percent_encode` is `!byte.is_ascii() ||
set.contains(byte)`, so non-ASCII is escaped regardless of which encode set is
in play, while `java.net.URI` treats non-ASCII path characters as legal and
leaves them alone. The raw/decoded comparison can only ever see what Java
itself escaped. Fixed in 39e02de39.
Rather than special-case non-ASCII, I derived the whole set the native
parser rewrites from the locked `url` 2.5 crate instead of reasoning about it.
A throwaway test parsed `hdfs://ns/pre<c>post/output` for every printable ASCII
`c` and compared `url.path()` with the input:
```
KEPT : !$%&'()*+,-.0123456789:;=@A-Z[\]^_a-z|~
ESCAPED : "#<>?`{}
NONASCII "café" -> "/caf%C3%A9/output"
NONASCII "日本語" -> "/%E6%97%A5%E6%9C%AC%E8%AA%9E/output"
NONASCII "🙂" -> "/%F0%9F%99%82/output"
NONASCII "e\u{301}" -> "/e%CC%81/output"
```
So nine ASCII characters plus every non-ASCII byte, and notably `%`, `[`,
`\`, `]`, `^` and `|` survive. The guard now declines anything in that set, on
the string actually handed to the native writer, which is `outputPath` itself
rather than the URI-decoded form. Deriving the set this way is also what keeps
it from over-declining: gating on "not plain ASCII alphanumerics" would have
refused `dt=2026-09-09/hour=17` and Spark's own
`_temporary/0/_temporary/attempt_.../part-0.parquet`, both of which are now
controls in the test.
I kept the Java comparison as a second condition rather than replacing it.
The native parser leaves `%` alone, so `50%off` never gets rewritten; it
reaches `Path::parse` as an invalid escape instead, and only Java's raw/decoded
difference sees it. The comment says that now, and no longer claims the
raw/decoded comparison detects the divergence on its own, which was the
overstatement you flagged.
Tests cover accented Latin both precomposed (U+00E9) and as `e` plus
combining acute (U+0301), CJK, an astral-plane emoji, non-ASCII in a nested
segment rather than the leaf, and the remaining escaped ASCII characters. They
are built from code points via `Character.toChars`, because scalastyle's
`NonASCIICharacterChecker` rejects non-ASCII source characters, which is also
why the scaladoc refers to U+00E9 rather than spelling it.
I checked the new condition is load-bearing rather than assuming it.
Disabling only `nativeEscaped` and leaving the tests in place:
```
- HDFS output paths needing URI escaping are declined at planning *** FAILED
***
escapedHdfsDestination(path).isDefined was false
expected hdfs://ns/café/output.parquet to be declined
```
With it enabled, all 43 `CometParquetWriterSuite` tests pass locally on the
default profile, which matches the count from the Spark 4.0/4.1/4.2 jobs.
`spotless:apply` and scalastyle are clean.
On the lint failure you spotted in the earlier round: the first run of the
suite here failed 43 of 43 for an unrelated reason worth recording, a stale
`libcomet` in `spark/target/classes` left by another branch's build. Maven
skips the copy when the destination is newer, so the suite silently ran against
a library without this branch's `parquet_writer.rs` and proto changes. Removing
it and rebuilding gave the clean run above.
--
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]
