Skip to content

Conversation

@SKY-ALIN
Copy link

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

@github-actions github-actions bot added documentation Improvements or additions to documentation sqllogictest SQL Logic Tests (.slt) functions Changes to functions implementation labels Jan 24, 2025
@Omega359
Copy link
Contributor

Thanks for your contribution! I've left some comments for your review.

@SKY-ALIN SKY-ALIN requested a review from Omega359 January 25, 2025 19:26
@alamb
Copy link
Contributor

alamb commented Jan 27, 2025

FYI @Omega359 noted this is quite similar to

@rluvaton
Copy link
Member

FYI Spark regex is not the same as Rust regex and can have different results

@SKY-ALIN SKY-ALIN requested review from Omega359 and rluvaton January 31, 2025 18:24
Copy link
Contributor

@Omega359 Omega359 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

&self.signature
}

fn return_type(&self, arg_types: &[DataType]) -> Result<DataType> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you implement return_type_from_args with marking nullable true only if the any of the input types are nullable (similiar to this #14841)

@Omega359
Copy link
Contributor

@SKY-ALIN - are you able to look into the above review comments?

@SKY-ALIN SKY-ALIN requested a review from Omega359 April 2, 2025 01:57
@Omega359
Copy link
Contributor

Omega359 commented Apr 2, 2025

I'll take a look at this later today @SKY-ALIN

@Omega359
Copy link
Contributor

Omega359 commented Apr 2, 2025

I spent some time today looking at this PR. Here are my thoughts:

  • This udf closely mirrors the duckdb functionality but it does not mirror the typical regex substring functionality found in postgres, mysql, sql server nor oracle
  • I do not think that datafusion should have both regexp_extract and regexp_substring functionality as the general consensus has been to try and mirror postgresql with respect to functionality if possible. Thus, if I was to choose to include this type of function it would be the regexp_substr, not regexp_extract. That is not to say that there isn't merit with regexp_extract but it may be best to have it in a repo such as datafusion-functions-extra or similar.

As a note the regexp_substr PR (#14323) is about to expire and the author looks to have submitted it and abandoned it. I think there is a number of ideas that you've implemented in this PR that could be incorporated into a new PR perhaps based on the above mentioned PR?

Just a thought if you are interested - I myself really do appreciate the work you've put into this PR ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation functions Changes to functions implementation sqllogictest SQL Logic Tests (.slt)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

regexp_extract func from Spark

4 participants