feat: issue_9285: port builtin reg function into datafusion-function-* (1/3 regexpmatch) #9329
Conversation
github-actions
bot
added
sql
There was a problem hiding this comment.
Thank you very much @Lordworms -- this PR looks great to me and it is very close I think. I left some small suggestions and then I think this will be ready to go
| use std::sync::Arc; | ||
|
|
||
| #[cfg(feature = "regex_expressions")] | ||
| macro_rules! invoke_on_array_if_regex_expressions_feature_flag { |
There was a problem hiding this comment.
I don't think these macros are needed here because the entire module is not included if the regex_expressions module is not compiled in.
| regexp::regexp_match(values, regex, Some(flags)) | ||
| .map_err(|e| arrow_datafusion_err!(e)) | ||
| } | ||
| other => internal_err!( |
There was a problem hiding this comment.
| other => internal_err!( | |
| other => exec_err!( |
|
|
||
| Ok(()) | ||
| } | ||
| // #[test] |
There was a problem hiding this comment.
It looks to me like regexp_match is already tested in https://github.com/apache/arrow-datafusion/blob/c439bc73b6a9ba9efa4c8a9b5d2fb6111e660e74/datafusion/sqllogictest/test_files/regexp.slt#L128-L215 so I think we can simply remove this test
Bonus points would be to port it to .slt
(there is no reason to leave it in and commented out)
There was a problem hiding this comment.
I would port it to .slt
| @@ -536,7 +536,7 @@ impl From<&protobuf::ScalarFunction> for BuiltinScalarFunction { | |||
| ScalarFunction::Lpad => Self::Lpad, | |||
| ScalarFunction::Random => Self::Random, | |||
| ScalarFunction::RegexpLike => Self::RegexpLike, | |||
| ScalarFunction::RegexpMatch => Self::RegexpMatch, | |||
| //ScalarFunction::RegexpMatch => Self::RegexpMatch, | |||
There was a problem hiding this comment.
| //ScalarFunction::RegexpMatch => Self::RegexpMatch, |
I think removing it entirely would be better
| @@ -1518,7 +1518,7 @@ impl TryFrom<&BuiltinScalarFunction> for protobuf::ScalarFunction { | |||
| BuiltinScalarFunction::Random => Self::Random, | |||
| BuiltinScalarFunction::Uuid => Self::Uuid, | |||
| BuiltinScalarFunction::RegexpLike => Self::RegexpLike, | |||
| BuiltinScalarFunction::RegexpMatch => Self::RegexpMatch, | |||
| //BuiltinScalarFunction::RegexpMatch => Self::RegexpMatch, | |||
There was a problem hiding this comment.
| //BuiltinScalarFunction::RegexpMatch => Self::RegexpMatch, |
|
Got it, I will fix this later today |
Lordworms
force-pushed
the
issue_9285_regx_match
branch
from
13a7425 to
c3e3a87
Compare
…* crate (1/3: RegexpMatch part)
Which issue does this PR close?
#9328
Closes #.
Rationale for this change
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?