andygrove commented on PR #5415: URL: https://github.com/apache/datafusion-comet/pull/5415#issuecomment-5441731820
> **Note on this review:** this was generated by an LLM (Claude Code) at my request while I worked through a review backlog. I have not verified the individual findings myself. Please treat everything below as suggestions to evaluate rather than as authoritative review feedback, and push back on anything that is wrong or already handled. This is impressive work. A recursive-descent parser implementing an explicit whitelist is much better than the substring matching this replaces, the rejected list reads like someone actually enumerated where Java and Rust diverge rather than guessing, and `CometRegexParitySuite` comparing against `java.util.regex.Pattern.find` over the full admitted corpus is the right shape of test. Being explicit that this is "not a formal proof of equivalence" is honest and appreciated. Four things. **The whitelist needs fuzzing, not just a corpus** The safety property here is: every pattern the parser admits behaves identically in Rust and Java. A fixed corpus tests the patterns you thought of. The bug that matters is the one where the parser admits something you did not think of, and then the result is silently wrong rather than an error. Would you add a generative test: produce random patterns from a grammar (including plenty that should be rejected), run them through `CometRegex`, and for every admitted pattern compare Java's `find` against the Rust kernel over a set of random subjects including non-ASCII, newlines, and empty strings? A few thousand iterations of that would find parser gaps that no hand-written corpus will. It could run as a nightly rather than on every PR if it is slow. **How much of the real world does the subset cover?** `.`, `^`, `$`, `\d`, `\w`, and `\s` are all rejected, and between them they appear in most regexes anyone actually writes. So it would be useful to know what fraction of realistic patterns the whitelist admits. Even a rough number from a corpus you have to hand, or from the regex-heavy TPC-DS queries, would tell us whether 360 lines of parser is buying meaningful acceleration or mostly covering patterns that are already cheap. That is not an argument against the PR. It is an argument for putting the number in the description, because it determines whether the follow-up work (`(?-u)` rewriting to admit `.` and the classes) is urgent or not. **Where do 256, 4096, and 32 come from?** The compile-budget gates are three magic numbers. `regex`'s own default size limit is 10 MB of compiled program, so presumably these were chosen to stay well under it, but the derivation is not written down. Could the constants carry a comment explaining what each one bounds and roughly what compiled size it corresponds to? Otherwise the next person who wants to raise one has no basis for deciding whether it is safe. **Coupling to the `regex` crate version** The whitelist encodes assumptions about how the `regex` crate behaves today. A future `regex` bump could change the semantics of an admitted pattern, and nothing in the build would notice. `CometRegexParitySuite` running in both PR workflows is good, but does the parity suite actually exercise the Rust kernel through native execution, or does it compare Java against expected values? If the former, a `regex` upgrade would break it, which is the behavior we want. If the latter, it would be worth adding at least one test that would fail on a semantics change upstream. **One process note** `CometRegExpBenchmark` is modified but the description does not include numbers. What is the speedup for an admitted pattern versus the dispatcher? That is the payoff for all of this and it should be in the description. -- 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]
