You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "andygrove (via GitHub)" <gi...@apache.org> on 2026/04/29 21:59:45 UTC
[PR] feat: support Spark expression `regexp_extract` [datafusion-comet]
andygrove opened a new pull request, #4146:
URL: https://github.com/apache/datafusion-comet/pull/4146
## Which issue does this PR close?
<!-- Closes #. -->
N/A
## Rationale for this change
`regexp_extract` is a common Spark SQL string function used to pull substrings out of input strings via regex capture groups. Adding native support lets queries that use it stay in Comet instead of falling back to Spark.
## What changes are included in this PR?
- New Rust UDF `spark_regexp_extract` in `native/spark-expr/src/string_funcs/regexp_extract.rs`, backed by the `regex` crate. Handles Utf8 and LargeUtf8 inputs (array and scalar), idx defaults to 1, idx=0 returns the whole match, no match returns the empty string, an unmatched optional group returns the empty string, null input returns null, and an out-of-range idx returns an execution error.
- Registration of `regexp_extract` in `comet_scalar_funcs.rs`.
- New `CometRegExpExtract` serde mapping `RegExpExtract` to the native UDF. Reported as `Incompatible` because the Rust regex engine has different semantics from Java's regex engine (POSIX classes, look-around, possessive quantifiers, etc.). Users opt in via `spark.comet.expression.RegExpExtract.allowIncompatible=true`. Falls back when the pattern or `idx` is non-literal.
## How are these changes tested?
- 9 Rust unit tests in `regexp_extract.rs` covering basic group extraction, idx=0/default idx, null subject, null pattern, unmatched optional group, out-of-range idx, negative idx, and invalid regex.
- Two new Comet SQL test files:
- `regexp_extract.sql` verifies that the expression falls back to Spark by default.
- `regexp_extract_enabled.sql` exercises the happy path with `allowIncompatible=true`, including default and explicit idx, idx=0, no-match, null input, optional unmatched groups, anchors, and an all-literal expression. Run under a `ConfigMatrix` for both dictionary-encoded and plain Parquet input.
--
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] feat: add native implementations of `regexp_extract` and `regexp_extract_all` [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove merged PR #4146:
URL: https://github.com/apache/datafusion-comet/pull/4146
--
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] feat: add native implementations of `regexp_extract` and `regexp_extract_all` [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on PR #4146:
URL: https://github.com/apache/datafusion-comet/pull/4146#issuecomment-4675833069
@mbutrovich I added microbenchmarks. It seems that there is justification to offer native-but-incompatible versions.
--
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] feat: add native implementations of `regexp_extract` and `regexp_extract_all` [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on PR #4146:
URL: https://github.com/apache/datafusion-comet/pull/4146#issuecomment-4672660850
Thanks for the careful review @mbutrovich. Pushed `367e07cd3` which addresses #2 and #3:
**#3 — `LargeUtf8` result type.** Confirmed your hunch. Both `regexp_extract::extract_array` and `regexp_extract_all::extract_all_array` are now hardwired to a `StringBuilder` (i32 offsets) regardless of input width, so a `LargeUtf8` subject still yields a `StringArray` matching `RegExpExtract.dataType` = `StringType`. Doc comment on each function explains the constraint. Added two regression tests (`large_utf8_subject_returns_utf8_array` in `regexp_extract.rs`, `large_utf8_subject_returns_inner_utf8` in `regexp_extract_all.rs`) that downcast the result to `StringArray` and would fail loudly if either UDF ever drifts back.
**#2 — Rust dedup.** New `regexp_extract_common.rs` with a `parse_args(fn_name, args)` helper that returns `ParsedArgs::Parsed { regex, group_idx, subject }` or `ParsedArgs::NullResult { len }`. Each UDF now starts with a one-match expression and is left with just its own per-row loop and per-function null shape (scalar `Utf8(None)` for extract, scalar/array `ListArray` of nulls for extract_all). Net diff is +1 file, ~120 lines removed across the two UDFs.
**#1 — `CometRegExpBase` on the Scala side.** The two motivations you cited (shared `regexp` allow-incompat key, `RegExp.isSupportedPattern` upgrading safe patterns to `Compatible`) were both removed by #4239 — that PR made all four of `CometRLike`, `CometRegExpReplace`, `CometRegExpExtract`, `CometRegExpExtractAll` return `Compatible()` and rely on per-class `getExprConfigName(expr)` for the allow-incompat key, with the codegen dispatcher as the byte-exact fallback. So your proposed `regexSupportLevel` (which would return `Incompatible` for unsafe patterns) would be a regression — those patterns now route through the dispatcher and are byte-exact.
The structural dedup itself is still real — all four serdes have a "`if isExprAllowIncompat AND nativeSupported THEN serialize ELSE emitJvmCodegenDispatch`" body. I'd rather scope that to a follow-up that can touch all four together (the `RLike` and `RegExpReplace` rewrites are out of this PR's lane), and tracked separately since the surface area — what `nativeSupported` means per-expression, what gets serialized — varies. Happy to file that issue if you agree, or leave a TODO in the code.
--
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] feat: add native implementations of `regexp_extract` and `regexp_extract_all` [datafusion-comet]
Posted by "mbutrovich (via GitHub)" <gi...@apache.org>.
mbutrovich commented on PR #4146:
URL: https://github.com/apache/datafusion-comet/pull/4146#issuecomment-4567844694
Thanks for tackling these @andygrove, the support-level handling and the SQL test shape (including the explicit `ignore` for lookahead) look great.
A few things I wanted to ask about:
### Could we lift this into a `CometRegExpBase`?
`CometRLike`, `CometRegExpReplace`, and the two new ones all carry the same Rust-vs-Java caveat, and all four take a literal pattern as their first regex arg. `RLike` and `RegExpReplace` already do two things the new pair doesn't:
1. They use the shared `regexp` allow-incompat key (`CometConf.isExprAllowIncompat("regexp")`), so the user opts in once for the whole family. The new PR introduces per-class `RegExpExtract.allowIncompatible` / `RegExpExtractAll.allowIncompatible` flags, which diverges.
2. They call `RegExp.isSupportedPattern(pattern)` to upgrade safe patterns to `Compatible` rather than always being `Incompatible`.
Worth folding all four into a common base?
```scala
abstract class CometRegExpBase[T <: Expression] extends CometExpressionSerde[T] {
protected def patternOf(expr: T): Expression
protected def isPatternSafe(expr: T): Boolean =
patternOf(expr) match {
case Literal(p, DataTypes.StringType) => RegExp.isSupportedPattern(p.toString)
case _ => false
}
override def getIncompatibleReasons(): Seq[String] = Seq(
"Uses Rust regexp engine, which has different behavior to Java regexp engine")
protected def regexSupportLevel(expr: T): SupportLevel =
patternOf(expr) match {
case _: Literal if isPatternSafe(expr) => Compatible()
case _: Literal => Incompatible()
case _ => Unsupported(Some("Only scalar regexp patterns are supported"))
}
}
```
`CometRegExpExtract` / `CometRegExpExtractAll` would then add their `idx`-must-be-literal check on top of `regexSupportLevel`, and the existing `CometRLike` / `CometRegExpReplace` lose their bespoke copies. That also gets the new pair onto the shared `regexp` allow-incompat key, which I think is what users would expect.
### Same idea on the Rust side?
`regexp_replace` is delegated upstream and `RLike` is its own `PhysicalExpr`, so there's no big cross-family dedup available, but within this PR `regexp_extract` and `regexp_extract_all` still share their arg parsing, regex compile, group-count validation, `subject_len`, `null_result`, and subject dispatch verbatim. A small shared helper that returns the parsed `(regex, group_idx, subject)` tuple (or an early-exit null result) would leave each UDF as just its own per-row loop:
```rust
pub(super) struct ParsedArgs<'a> {
pub regex: Regex,
pub group_idx: usize,
pub subject: &'a ColumnarValue,
}
pub(super) enum ParseResult<'a> {
Parsed(ParsedArgs<'a>),
NullResult(ColumnarValue), // null pattern or null idx short-circuit
}
pub(super) fn parse_regexp_extract_args<'a>(
fn_name: &'static str,
args: &'a [ColumnarValue],
) -> DataFusionResult<ParseResult<'a>> { ... }
```
### `LargeUtf8` result type in `regexp_extract`
`extract_array::<O>` is generic over the offset width, so a `LargeUtf8` input array yields a `LargeStringArray`, but the serde declares the return type as `expr.dataType` (`Utf8`). Is that intentional, or worth always returning `Utf8` (the scalar branch already does)? Something like:
```rust
DataType::Utf8 | DataType::LargeUtf8 => {
let strings: &dyn Array = array.as_ref();
Ok(ColumnarValue::Array(extract_array(strings, ®ex, group_idx)))
}
```
with `extract_array` always producing a `StringArray` (i32 offsets), since `&str` slices are width-agnostic. `regexp_extract_all` looks consistent here since its value builder is always `StringBuilder`.
Possibly related: I didn't see a Rust unit test for a `LargeUtf8` subject in either file, would be nice to lock that branch down.
--
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