andygrove commented on PR #5350: URL: https://github.com/apache/datafusion-comet/pull/5350#issuecomment-5286508833
Third run of the skill, added here: **[#5351: Run `rlike` natively by default for patterns that are provably Java-regex equivalent](https://github.com/apache/datafusion-comet/issues/5351)**, recorded under `## rlike` in `predicate_funcs.md`. This is the first run to reach the `Hybrid` sub-pool, which only became visible after the pool-widening fix in #5348, and it turned out to be the strongest candidate assessed so far: - **The kernel already exists.** `rlike` runs on the dispatcher not because there is no native path but because `CometRLike.nativeApplicable` checks only *whether the pattern is a literal*, never what the pattern contains. So `^abc[0-9]+$`, which both engines agree on, is treated exactly like `(?<=foo)bar`, which the Rust engine cannot compile at all. The proposal is a plan-time pattern oracle mirroring `CometCast`, so the provably-equivalent subset becomes the default. - **The divergence list is already written down.** `compatibility/regex.md` enumerates every construct that differs, and each one is detectable by inspecting a literal pattern. The issue turns that prose into a reject list. - **Measured, from the emitted kernel.** 7 allocation sites per non-null row, 4 of them inside a single `Pattern.matcher()` call: the `Matcher` plus its `groups` (`int[20]` minimum), `locals`, and `localsPos` arrays, on top of `UTF8String.toString()`'s `getBytes` copy, `String` header, and internal `byte[]`. I also checked what is *not* a cost: the `Pattern` is compiled once into mutable state, and the input read is zero-copy, so neither can be claimed as upside. - **It engages with the prior decision rather than ignoring it.** #4310 concluded the Rust *engine* can never be fully Java-compatible, which is true. This is a per-pattern decision, an axis that discussion did not consider. Saying so in the issue seemed better than letting a reviewer discover the overlap. - **Multiplier.** The same oracle unlocks `regexp_replace`, `split`, `regexp_extract`, and `regexp_extract_all`, which are all `NativeOptInAvailable` for the same reason. One thing I deliberately did not measure: the end-to-end dispatcher-versus-native A/B, which needs a release build of the native library. Since both paths already exist it is cheap for whoever implements this (`CometRegExpBenchmark` with and without `allowIncompatible=true`), and the issue puts it in the acceptance criteria rather than guessing at a number. This PR now carries three assessments (`unbase64`, `sequence`, `rlike`). Still draft, since they are only worth landing if the assessments they record look sound. -- 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]
