andygrove commented on PR #5415:
URL:
https://github.com/apache/datafusion-comet/pull/5415#issuecomment-5443268056
My main worry going in was that the whitelist only inspects the pattern, so
I wanted to know whether the subject data could still drive Java and Rust apart
— control characters and exotic whitespace especially. I spent some time
testing that rather than guessing, and I think you're fine. I ported
`CometRegex` to Java so I could fuzz it cheaply, then compiled your actual
Scala file against stub `SupportLevel` types and checked both on 300k random
strings over a metachar-heavy alphabet to confirm the port was exact (no
decision mismatches, ~28k admitted). Running the analyzer-admitted patterns
through `java.util.regex` and the `regex` crate against subjects built from
every C0 control, DEL, NEL, NBSP, soft hyphen, all the Unicode space
separators, U+2028/9, BOM, ZWSP, U+FFFD, emoji and U+10FFFF, I got zero
divergences over about 68M pattern/subject pairs, and compile status agreed
everywhere too, so there's no admitted pattern where Java throws and Rust
doesn't or the other way ro
und. The reason it holds looks structural to me: every whitespace and
line-terminator disagreement I could find lives in a construct you reject, and
the one data-sensitive construct that survives — the negated class — is
code-point-based on both sides.
The one thing the pattern whitelist can't reach is invalid UTF-8 in the
subject. Rust's `regex` treats invalid bytes as unmatchable while Java decodes
them to U+FFFD first, so `[^0-9]` against a lone `0xC3` is true on Spark and
false natively. `cast_binary_to_string` does a lossy decode so that path is
safe, but I don't see UTF-8 validation in the native Parquet reader. This is
pre-existing for every native string kernel and I'm not asking you to fix it
here, but `rlike` is now default-on, so could you add a line to
`compatibility/regex.md` noting that the subset assumes well-formed UTF-8?
Two smaller things. The parity suite's subject list has `\n`, `\r`, `\r\n`,
U+0085, U+2028 and U+2029 but not tab, VT, FF, NUL, DEL, NBSP or BOM — the
behavior is right, it's just that those are the characters a reader is most
likely to want covered, so would you mind adding them? And `nativeApplicable`
got widened from `Literal(_, DataTypes.StringType)` to `Literal(v: UTF8String,
_: StringType)`, which matches any collation. `provablyCompatible` guards
collation but the `allowIncompat && nativeApplicable(expr)` branch in `convert`
doesn't, so `allowIncompatible=true` on a UNICODE-collated `rlike` now goes
native where it previously hit the dispatcher. Should the collation guard move
into `nativeApplicable`?
The other thought is that the safety argument rests entirely on `regex`
crate semantics, and the only parity coverage is 30 patterns going through
Spark SQL. Would a table of (pattern, subject, expected) triples generated
offline from Java, as a unit test in `rlike.rs`, be worth adding? That would
catch a semantics change on a crate upgrade for almost nothing.
Last one, and for a follow-up rather than this PR: leading `^` looks safe to
admit. Java's `^` outside MULTILINE and Rust's `^` without `(?m)` both anchor
to input start only — it's `$` that differs, since Java matches it before a
final line terminator. I fuzzed `^`-prefixed variants of all the admitted
patterns against the same control-heavy subjects and saw no divergences. Given
how common `^prefix` is in real workloads, that seems like a decent chunk of
coverage left on the table.
Nice work on this. The recursive-descent whitelist instead of substring
matching is the right call, and the compile budget errs in the safe direction —
collapsing `{0}` to 1 over-estimates rather than under-estimates what Rust
actually expands.
--
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]
