You are viewing a plain text version of this content. The canonical link for it is here.

Posted to github@datafusion.apache.org by "dwsmith1983 (via GitHub)" <gi...@apache.org> on 2026/09/01 14:59:35 UTC

[PR] perf: compile user regex patterns once per planned expression [datafusion-comet]

dwsmith1983 opened a new pull request, #5612:
URL: https://github.com/apache/datafusion-comet/pull/5612

   ## Which issue does this PR close?
   
   No dedicated issue. Related to #4942, whose description says the remaining `Regex::new` calls were already hoisted into statics; these three user-pattern call sites were still compiling per batch on current main.
   
   ## Rationale for this change
   
   `regexp_extract`, `regexp_extract_all`, and `split` called `Regex::new` on the user's pattern inside the per-batch evaluation path, so every 8192-row batch paid a full regex compile. `rlike` in the same crate already compiles once at plan time; these three could not take that exact shape because they are scalar functions created by name, and the pattern only arrives per invocation as a scalar argument.
   
   ## What changes are included in this PR?
   
   Each planned expression now owns a one-slot `PatternCache` (new `string_funcs/pattern_cache.rs`): compile on first use, reuse while the pattern string is unchanged, recompile if it ever differs (split's serde does not require a literal pattern, so the cache tolerates changes rather than assuming a constant). `Regex` clones share the compiled program, so handing out clones per batch is an Arc bump. Error messages are byte-identical and an invalid pattern still fails at the same phase as before.
   
   Numbers on an M-series mac: criterion `regexp_extract` goes from 862us to 705us per 8192-row batch (about 18% faster), and a small-batch run (512 rows, 5000 batches) is 2.1x faster since compile cost is amortized over fewer rows. The split bench is flat because its case uses a literal delimiter, which takes the non-regex fast path. One known unknown worth stating: the cache uses a Mutex and the benches are single-threaded, so contention under DataFusion's intra-task parallelism is unmeasured. The fast path is a lock, a string compare, and a clone, so it should be negligible, and the lock also prevents duplicate compiles on a cold cache.
   
   ## How are these changes tested?
   
   Seven new tests: three cache unit tests (compile-once, recompile-on-change, invalid pattern does not poison the slot), three multi-batch tests pinning one compile across batches per function via a test-only counter, and one pinning that an invalid split pattern still errors at evaluation. Full crate suites pass (670 spark-expr, 212 core), clippy with warnings denied and fmt are clean, and the Scala side was exercised through CometStringExpressionSuite (33 tests, includes the native split path) and CometRegExpJvmSuite (46 tests).
   


-- 
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: github-unsubscribe@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org

Re: [PR] perf: compile user regex patterns once per planned expression [datafusion-comet]

Posted by "sunchao (via GitHub)" <gi...@apache.org>.

sunchao commented on PR #5612:
URL: https://github.com/apache/datafusion-comet/pull/5612#issuecomment-5508929795

@dwsmith1983 Thanks for covering the requested matrix. Could you attach the runnable harness/commands, exact baseline and PR commit SHAs, dependency/build settings, and per-cell results for both replicates, including the regressing shared-instance cases?

Per-task plan ownership does not rule out sharing within a task. Source inspection shows that Comet passes sort-key expressions directly to `SortExec`. In DataFusion 54.1.0, `ExternalSorter::in_mem_sort_stream` uses `spawn_buffered` for multiple retained batches once the reservation reaches `sort_in_place_threshold_bytes`. The cloned orderings retain the same expression/UDF, which can then be evaluated concurrently on Comet's multithread runtime. Could you add a native sort case with `regexp_extract_all` directly in the sort key, no LIMIT, 8192-row batches, and enough unsorted input to reach that branch? A one-/eight-worker comparison, with the native plan and evidence of overlapping calls to the same UDF, would test whether the adverse control matters here. A precomputed regex column would not exercise that sharing. This is source evidence for the path, not a reproduced end-to-end slowdown.

Could you also revisit the scratch-pool attribution? `PatternCache::get_or_compile` returns an owned `Regex` clone, and the pinned regex-automata 0.4.16 `Regex::clone` creates a fresh scratch-cache pool. Sharing the compiled program is not sharing that scratch pool. The reported slowdown may still be real, but its cause needs the harness or profiling evidence. I have not independently rerun these timings.

--
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: github-unsubscribe@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org

Re: [PR] perf: compile user regex patterns once per planned expression [datafusion-comet]

Posted by "dwsmith1983 (via GitHub)" <gi...@apache.org>.

dwsmith1983 commented on PR #5612:
URL: https://github.com/apache/datafusion-comet/pull/5612#issuecomment-5536242508

   Thanks, that double search was real and it reproduced here at +24 percent and +43 percent on your two cases. Fixed in ea8c0e369: the loop now drives `captures_read_at` alone with one reused `CaptureLocations`, taking the overall match from group 0, so each match is searched once and there is still no per-match allocation. The empty-match rule mirrors `Searcher::try_advance` in regex-automata 0.4.16 (an empty match ending where the previous one ended advances one byte and searches again), and an equivalence test compares the helper against `captures_iter` across 12 patterns including `a*`, `\\b`, and `(?:)`, 12 haystacks including CJK and emoji, and groups 0, 1, 2, and an out-of-range index.
   
   Your two cases are now in `benches/regexp_extract_all.rs`. Criterion medians on this machine, base is main 55ae4f20:
   
   | case | base | before fix | after fix | after vs base |
   |---|---:|---:|---:|---:|
   | 8,192 rows of 123-456-789-123, (\\d+), group 1 | 1.93 ms | 2.39 ms | 1.44 ms | -26% |
   | 512 rows of 8 KB matched by (a+), group 1 | 59.2 ms | 84.5 ms | 48.2 ms | -19% |
   | 524,288 rows, no nulls (existing case) | 193.5 ms | 250.2 ms | 163.9 ms | -15% |
   
   Full table is in the commit's bench file output; every case is below base after the fix.


-- 
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: github-unsubscribe@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org

Re: [PR] perf: compile user regex patterns once per planned expression [datafusion-comet]

Posted by "andygrove (via GitHub)" <gi...@apache.org>.

andygrove merged PR #5612:
URL: https://github.com/apache/datafusion-comet/pull/5612


-- 
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: github-unsubscribe@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org

Re: [PR] perf: compile user regex patterns once per planned expression [datafusion-comet]

Posted by "dwsmith1983 (via GitHub)" <gi...@apache.org>.

dwsmith1983 commented on PR #5612:
URL: https://github.com/apache/datafusion-comet/pull/5612#issuecomment-5508444878

   Ran the benchmark on a 10 core Apple M5 (4P + 6E), comparing this branch against the base commit it sits on. The harness builds the UDFs through `create_comet_physical_fun` and calls `invoke_with_args` with the pattern arriving as a scalar argument on every invoke, which is the native path and the reason the cache exists. Matrix: regexp_extract, regexp_extract_all, and split with a regex delimiter (`[,;|]+`), shared and per worker UDF instances, 1/2/4/8 workers, 512 and 8192 row batches, warm and alternating pattern regimes, 4M rows per cell, two full replicates. Outputs were verified byte identical between main and this PR in every cell.
   
   Warm regime, per worker instances (matches real plans, where the pattern is a literal and each task gets its own expression instance):
   
   | function | workers | rows/batch | main Mrows/s | this PR Mrows/s | change |
   |---|--:|--:|--:|--:|--:|
   | regexp_extract | 1 | 512 | 6.9 | 17.9 | +159% |
   | regexp_extract | 8 | 512 | 5.8 | 62.8 | +986% |
   | regexp_extract | 8 | 8192 | 79.3 | 111.9 | +41% |
   | regexp_extract_all | 8 | 512 | 3.2 | 6.5 | +103% |
   | split | 8 | 512 | 25.6 | 30.2 | +18% |
   | split | 8 | 8192 | 31.7 | 31.8 | 0% |
   
   Main anti-scales on small batches: 8 threads run slower than 1 because every thread recompiles the pattern per batch and the compiles hammer the allocator. This PR scales near linearly. Allocations per 512 row batch for regexp_extract drop from 988 to 82 (the compile alone is roughly 900 allocations and 0.7 MB). Per batch latency follows the same shape, for example 699us mean / 1271us p99 down to 62us / 106us in the 8 worker 512 row cell.
   
   Worst case for the one slot cache, a pattern that alternates on every single invoke: within 2 percent of main across all three functions and both batch sizes, since the miss path pays the same compile main always pays plus an uncontended mutex. Cold first invoke on a fresh instance is also unchanged (for example 415us on main vs 403us here for regexp_extract on 8192 rows).
   
   One honest caveat: an artificial control where a single UDF instance is shared across 8 threads simultaneously regresses regexp_extract_all on 8192 row batches by 6 to 29 percent. The threads contend on the shared compiled Regex's internal scratch pool in that setup, while main sidesteps it by compiling privately per batch, which is the same behavior causing the anti-scaling above. That configuration does not occur in Comet since each task deserializes its own plan and gets its own expression instance, and regexp_extract and split win in shared mode anyway.
   


-- 
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: github-unsubscribe@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org

Re: [PR] perf: compile user regex patterns once per planned expression [datafusion-comet]

Posted by "sunchao (via GitHub)" <gi...@apache.org>.

sunchao commented on PR #5612:
URL: https://github.com/apache/datafusion-comet/pull/5612#issuecomment-5536044433

Found **one P2 performance regression** on `f0fe7b29`, reviewed against `55ae4f20`.

**`regexp_extract_all` searches each match twice.** At [regexp_extract_all.rs:118](https://github.com/apache/datafusion-comet/blob/f0fe7b29de4f60a836dbcbf6a2c1b4b22bf7638e/native/spark-expr/src/string_funcs/regexp_extract_all.rs#L118-L119), `find_iter` locates the match, then `captures_read_at` searches again. On matched inputs, that extra work can outweigh the cache savings.

My independent reproduction measured:

| Native input | Base | PR | Slowdown |
|---|---:|---:|---:|
| 8,192 rows of `123-456-789-123`, pattern `(\d+)`, group 1 | 4.75 ms | 6.50 ms | **37%** |
| 512 rows of 8KB strings containing `a`, pattern `(a+)`, group 1 | 54.60 ms | 79.01 ms | **45%** |

Three runs agreed. These measurements include baseline compilation, PR cache lookup/cloning, and Arrow output construction/destruction. They use the exact source and pinned dependencies, without allocation-counting instrumentation. They measure native function calls, not whole Spark queries.

**The cache lifetime itself is correct:** production reuses the captured cache; `call_raw` remains test-only.

Validation passed:

- 114 string-function tests.
- Eight cache/concurrency tests.
- 32,154 comparisons of base/head results and errors.
- Current CI: 57 successful checks, eight pending, six skipped, no failures. [Checks](https://github.com/apache/datafusion-comet/pull/5612/checks)

I would retain the cache optimization and revise the capture iteration before merging.

**[P2] Avoid searching every regex match twice**

The find_iter/captures_read_at combination regresses complete native regexp_extract_all calls on matched inputs. Three optimized exact-source runs, including an independent repeat, measured about 37–39% more time for 8,192 rows of '123-456-789-123' with pattern '(\d+)' and group 1, and 44–45% more time for 512 rows of 8KB strings matched by '(a+)'. Measurements include baseline compilation, the new cache lookup/clone, and Arrow output construction/destruction. Please preserve the cache benefit while avoiding this repeated matching work, and cover these cases in benchmarks.

--
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: github-unsubscribe@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] perf: compile user regex patterns once per planned expression [datafusion-comet]

Posted by "dwsmith1983 (via GitHub)" <gi...@apache.org>.

dwsmith1983 commented on PR #5612:
URL: https://github.com/apache/datafusion-comet/pull/5612#issuecomment-5521476976

   The two failing legs here are infrastructure, not the change: spark-sql-sql_hive-3 died fetching compiler-bridge from Maven Central (java.net.SocketException, Network is unreachable) while compiling Spark itself, and the Iceberg 1.11 leg could not fetch the shadow plugin from plugins.gradle.org, same flake that hit #5568 last week. Neither reached any Comet code. Could someone with access rerun those two jobs when convenient?


-- 
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: github-unsubscribe@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org

Re: [PR] perf: compile user regex patterns once per planned expression [datafusion-comet]

Posted by "dwsmith1983 (via GitHub)" <gi...@apache.org>.

dwsmith1983 commented on PR #5612:
URL: https://github.com/apache/datafusion-comet/pull/5612#issuecomment-5511332599

Thanks for pushing on all three points. You were right on both technical claims, so taking them in order.

Harness and raw data: https://gist.github.com/dwsmith1983/e46e22c1c594b4f5120515c773f2b3ef has the full harness source, exact build and run commands, both commit SHAs, toolchain and dependency versions, and per-cell CSVs for both replicates including the regressing shared-instance cells.

Sort path: your reading of ExternalSorter checks out and the repro confirms it. With the exact plan shape Comet produces (SortExec, no fetch, single partition, 128 x 8192-row batches so the reservation is well past sort_in_place_threshold_bytes), a tracking shim around one UDF instance measured max 9 concurrent in-flight evaluations at 8 runtime workers, and even 2 at 1 worker since the merge evaluates concurrently with a spawned sort task. On that path regexp_extract_all as the sort key was 1.45x slower than base at 1 worker and 2.2x at 8. regexp_extract as the key was parity to slightly faster.

Attribution: you were right that my scratch-pool explanation was wrong. Clone creates a fresh private pool (meta/regex.rs 1916-1926), so scratch state is never shared. The real mechanism, isolated in a micro benchmark in the gist, is per-row refcount traffic: captures_iter creates a Captures per row via create_captures, which is Captures::all(self.group_info().clone()), an Arc clone against the program-owned GroupInfo, plus one more Captures clone per match. With every thread holding clones of one compiled program, that single refcount cache line bounces across cores and caps throughput regardless of thread count. A variant with one clone per thread, no lock and no per-invoke clone still collapses identically, which rules out the mutex and the clone itself. regexp_extract is immune because it reuses one CaptureLocations across rows, and split never creates a Captures.

That pointed at the fix, now pushed: regexp_extract_all drives iteration with find_iter (identical span semantics, verified against the crate's shared iterator code and pinned with empty-match and multibyte edge tests) and resolves groups through captures_read_at into one CaptureLocations reused per batch, same as regexp_extract. Rerun results: the sort scenario goes from 2.2x slower to 8 percent faster than base at 8 workers, shared-instance and per-instance modes are now identical, and removing the per-match allocations lets the function scale near linearly to 8 workers (7.1 to 72 Mrows/s at 8 workers, where base and the previous head were both stuck near 7). Outputs stay byte identical across base and both head builds in every cell. Fix verification tables and CSVs are in the gist as well.

--
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: github-unsubscribe@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org