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: [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]
