Add regexp_extract func #14282
Add regexp_extract func #14282
Conversation
github-actions
bot
added
documentation
|
Thanks for your contribution! I've left some comments for your review. |
|
FYI @Omega359 noted this is quite similar to |
|
FYI Spark regex is not the same as Rust regex and can have different results |
| LargeUtf8 => LargeUtf8, | ||
| Utf8 => Utf8, | ||
| Utf8View => Utf8View, | ||
| Null => Null, |
There was a problem hiding this comment.
you say return type is null while you never returned null
There was a problem hiding this comment.
line 157
(None, _, _) | (_, None, _) | (_, _, None) => {
// If any of arguments is null, the result will be null too
builder.append_null();
}UPD: It's line 181 now
There was a problem hiding this comment.
This does not handle null array, there is a special type called NullArray
| Ok(match &arg_types[0] { | ||
| LargeUtf8 => LargeUtf8, | ||
| Utf8 => Utf8, | ||
| Utf8View => Utf8View, |
There was a problem hiding this comment.
Your implementation does not return UTF8 view but only return tug8 or large utf 8 (GenericStringBuilder)
There was a problem hiding this comment.
Indeed. To actually return Utf8View you would need to do something similar to what regexp replace does
There was a problem hiding this comment.
This is looking good.
- I may have missed it but I don't see tests for out of bounds indexes (negative #'s, index larger than match group count).
- The docs do not say whether the index is 0-indexed or 1-indexed.
- I would love to see a benchmark for this but that absolutely could be added as a followup PR
- The returning of Utf8View should be corrected - either just return Utf8/LargeUtf8 or fix the return to actually return utf8view if input is utf8view
My primary concern is there are two PR's for essentially the same functionality. I am not in a position to decide which should be included.
| Ok(match &arg_types[0] { | ||
| LargeUtf8 => LargeUtf8, | ||
| Utf8 => Utf8, | ||
| Utf8View => Utf8View, |
There was a problem hiding this comment.
Indeed. To actually return Utf8View you would need to do something similar to what regexp replace does
|
|
||
| #[user_doc( | ||
| doc_section(label = "Regular Expression Functions"), | ||
| description = "Extract a specific group matched by [regular expression](https://docs.rs/regex/latest/regex/#syntax). If the regex did not match, or the specified group did not match, an empty string is returned..", |
There was a problem hiding this comment.
| description = "Extract a specific group matched by [regular expression](https://docs.rs/regex/latest/regex/#syntax). If the regex did not match, or the specified group did not match, an empty string is returned..", | |
| description = "Extract a specific group matched by [regular expression](https://docs.rs/regex/latest/regex/#syntax). If the regex did not match, or the specified group did not match, an empty string is returned.", |
| let group_idx = idx_val as usize; | ||
| if group_idx < caps.len() { | ||
| // If specified group index really exists | ||
| if let Some(m) = caps.get(group_idx) { |
There was a problem hiding this comment.
If group_idx is 0 this would return the whole string, right?
Which issue does this PR close?
Closes #14280.
Rationale for this change
Adding more functions
What changes are included in this PR?
The implementation of regexp_extract based on Spark regexp_extract
Are these changes tested?
Yes
Are there any user-facing changes?
No