Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issue 25 : Adding support for regex_replace and regex_match #122

Merged
merged 8 commits into from
May 24, 2023

Conversation

edmondop
Copy link
Contributor

@edmondop edmondop commented May 15, 2023

Solves #25

This pull requests add regexp_match and regexp_replace as defined in DataFusion signature.

Please note that the DataFusion signature is different from the PostgresSQL for what concerns the regexp_replace. In particular, the start and N parameters defined below are not supported


regexp_match('foobarbequebaz', '(bar)(beque)') → {bar,beque}

regexp_replace ( string text, pattern text, replacement text, start integer, N integer [, flags text ] ) → text

Replaces the substring that is the N'th match to the POSIX regular expression pattern, or all such matches if N is zero; see [Section 9.7.3](https://www.postgresql.org/docs/current/functions-matching.html#FUNCTIONS-POSIX-REGEXP).

regexp_replace('Thomas', '.', 'X', 3, 2) → ThoXas

@edmondop edmondop marked this pull request as ready for review May 18, 2023 04:58
@edmondop edmondop changed the title DRAFT Issue 25 : Adding support for regex_replace and regex_match Issue 25 : Adding support for regex_replace and regex_match May 18, 2023
@edmondop edmondop marked this pull request as draft May 18, 2023 15:53
@edmondop edmondop force-pushed the issue-25 branch 4 times, most recently from d534dcc to 0cdc7c0 Compare May 22, 2023 22:19
@edmondop edmondop force-pushed the issue-25 branch 3 times, most recently from 00ee61a to fe4acd4 Compare May 23, 2023 16:12
@edmondop edmondop force-pushed the issue-25 branch 2 times, most recently from 0ec3b1b to 3d43b0c Compare May 23, 2023 21:44
@edmondop edmondop force-pushed the issue-25 branch 7 times, most recently from b143822 to d3db0a9 Compare May 24, 2023 02:21
@edmondop edmondop marked this pull request as ready for review May 24, 2023 02:48
Copy link
Contributor

@jacksonrnewhouse jacksonrnewhouse left a comment

Choose a reason for hiding this comment

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

Looks good, just a couple small notes and rebasing on master.

@@ -2041,8 +2085,7 @@ impl StringFunction {
})
}
}
StringFunction::RegexpMatch(_, _) => todo!(),
StringFunction::RegexpReplace(_, _, _, _, _) => todo!(),
StringFunction::RegexpReplace(_, _, _, _) => unreachable!(),
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd clarify this is the Some(_) in the fourth argument case here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed with 9889c37

use std::error::Error;

#[test]
pub fn test_regexp_match_is_correct() -> Result<(), Box<dyn Error>> {
Copy link
Contributor

Choose a reason for hiding this comment

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

In general you don't need to have return types on your tests. Either it panics or it passes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed with 9889c37

I am normally more used to work with functional errors rather than panics, that's why I naturally leaned on using results

@edmondop edmondop requested a review from jacksonrnewhouse May 24, 2023 22:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants