perf: compile user regex patterns once per planned expression

[dwsmith1983]

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).

[sunchao]

Reviewed b4d9152367c8a0233beaa8b9817a69c0969e0c11 against 8729f6e6adf7091e18a48670e790d4ba8fd41e51. No verified P1/P2 findings.

A focused check of the unchanged cache source passed six tests, including concurrent cold access, different-pattern replacement and invalid-pattern recovery. This was a cache-only check, not the full Comet/Spark suite. Current-head workflows report action_required. The earlier HEAD's green checks are not current-head validation.

Could you add a matched BASE/HEAD multithreaded benchmark with shared-UDF and per-worker controls, using 1/2/4/8 workers and 512/8192-row batches? Please cover cold and warm caches, alternating patterns, and a regex delimiter for split, and report throughput, batch latency and allocations while checking equal results and confirming the native path. Regex::clone() shares the compiled program but creates a fresh search-cache pool, so this would measure both contention and the per-batch clone cost.

[dwsmith1983]

@sunchao
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.

[sunchao]

@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.

[dwsmith1983]

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.

[sunchao]

Reviewed the capture-location update in 84f9ee006fc94fb592b61439d9d520d484af616d and the pinned benchmark follow-up. No new P1/P2 findings.

A focused regex-only comparison passed for 86 patterns and 1,512 strings, including empty matches, optional groups, anchors, word boundaries, and UTF-8 offsets. The benchmarked fix has the same relevant source as this HEAD, and the supplied sort results show recovery of the reported regression. Those timings are author-run evidence, not my measurements. I did not run the full Comet/Spark suite; the final CI snapshot had 37 passing checks, 28 queued/running, and 6 skipped.

[sunchao]

Rechecked d0fea134. The PR-only patch is byte-for-byte identical to the previous reviewed pair, and the head-to-head change exactly matches the base update. I checked the affected integration paths and found no new P1/P2. The existing approval stands.

The relevant regex code and dependencies are unchanged, so prior component evidence was reused without rerunning tests. This head has no check runs and three workflows awaiting approval.

[dwsmith1983]

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?

[sunchao]

Re-reviewed a7d2b8af394262030c35899b8d42e7ef8ac4206c against ef62b463. The regex implementation and dependencies are unchanged. I checked the incoming remote dictionary-decoding path at the regex input boundary and found no new P1/P2. The existing approval stands.

This was a source/integration review. The earlier regex-only probe remains applicable by source/dependency identity, but I did not rerun it or execute a Spark/Comet query or benchmark. The three current-head workflows still require authorization, with no executed check results reported.

[sunchao]

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

regexp_extract_all searches each match twice. At regexp_extract_all.rs:118, 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

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.

[dwsmith1983]

Thanks, that double search was real and it reproduced here at +24 percent and +43 percent on your two cases. Fixed in ea8c0e3: 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 55ae4f2:

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.