You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "mbutrovich (via GitHub)" <gi...@apache.org> on 2025/08/14 17:58:44 UTC
[PR] feat: better Utf8View support for `regexp_replace`, signature cleanup [datafusion]
mbutrovich opened a new pull request, #17195:
URL: https://github.com/apache/datafusion/pull/17195
## Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
-->
- Closes #.
## Rationale for this change
<!--
Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.
-->
#11667 added some `Uft8View` support for `regexp_replace`, but only for the first argument. The other args only support `Uft8`, which likely isn't a huge deal since they're probably short literals, but adding support for other string types makes it easier to simplify the signature. Systems like Comet (which may emit string literals as `Utf8View`) will also benefit from being able to use this function.
## What changes are included in this PR?
<!--
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
-->
- Change the signature to rely on the logical string type. Oddly enough, though there was logic for `LargeUtf8` the signature did not list it, so not sure this worked end-to-end for `LargeUtf8`.
- Modify `fetch_string_arg` to support other string types besides `Utf8`.
- Remove logic in `return_type` that mentioned binary types. This function does not support binary types.
- Add unit tests for other string types.
- Add end-to-end tests for other string types.
## Are these changes tested?
<!--
We typically require tests for all PRs in order to:
1. Prevent the code from being accidentally broken by subsequent changes
2. Serve as another way to document the expected behavior of the code
If tests are not included in your PR, please explain why (for example, are they covered by existing tests)?
-->
Yes, there are new tests. See above.
## Are there any user-facing changes?
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
<!--
If there are any breaking changes to public APIs, please add the `api change` label.
-->
The signature for `regexp_replace` changed, but it was wrong before (did not have `LargeUtf8`).
--
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: support `Utf8View` for more args of `regexp_replace` [datafusion]
Posted by "mbutrovich (via GitHub)" <gi...@apache.org>.
mbutrovich commented on PR #17195:
URL: https://github.com/apache/datafusion/pull/17195#issuecomment-3192332664
I'm afraid I might have lost the plot with all of those string args. I looked at other string functions to see if a huge `match` is how we handle this, but the worst I could find was 2 args, not 4.
--
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: support `Utf8View` for more args of `regexp_replace` [datafusion]
Posted by "mbutrovich (via GitHub)" <gi...@apache.org>.
mbutrovich commented on code in PR #17195:
URL: https://github.com/apache/datafusion/pull/17195#discussion_r2282955561
##########
datafusion/functions/src/regex/regexpreplace.rs:
##########
@@ -94,14 +98,30 @@ impl Default for RegexpReplaceFunc {
impl RegexpReplaceFunc {
pub fn new() -> Self {
- use DataType::*;
+ use TypeSignature::*;
+ use TypeSignatureClass::*;
Self {
signature: Signature::one_of(
vec![
- TypeSignature::Exact(vec![Utf8, Utf8, Utf8]),
Review Comment:
Actually, if I reorder them into the precedence that I want...
```
signature: Signature::one_of(
vec![
Exact(vec![Utf8View, Utf8View, Utf8View]),
Exact(vec![LargeUtf8, LargeUtf8, LargeUtf8]),
Exact(vec![Utf8, Utf8, Utf8]),
Exact(vec![Utf8View, Utf8View, Utf8View, Utf8View]),
Exact(vec![LargeUtf8, LargeUtf8, LargeUtf8, LargeUtf8]),
Exact(vec![Utf8, Utf8, Utf8, Utf8]),
],
Volatility::Immutable,
),
```
I get...
```
External error: 1 errors in file /Users/matt/git/datafusion/datafusion/sqllogictest/test_files/string/string_view.slt
1. query result mismatch:
[SQL] EXPLAIN SELECT
REGEXP_REPLACE(column1_utf8view, '^https?://(?:www\.)?([^/]+)/.*$', '\1') AS k
FROM test;
[Diff] (-expected|+actual)
logical_plan
- 01)Projection: regexp_replace(test.column1_utf8view, Utf8("^https?://(?:www\.)?([^/]+)/.*$"), Utf8("\1")) AS k
+ 01)Projection: regexp_replace(test.column1_utf8view, Utf8View("^https?://(?:www\.)?([^/]+)/.*$"), Utf8View("\1")) AS k
02)--TableScan: test projection=[column1_utf8view]
at /Users/matt/git/datafusion/datafusion/sqllogictest/test_files/string/string_view.slt:801
```
Which might be okay? Defining the literals as `Utf8View` is pretty harmless.
edit: nope that bombs out for:
```
External error: 1 errors in file /Users/matt/git/datafusion/datafusion/sqllogictest/test_files/regexp/regexp_replace.slt
1. query failed: DataFusion error: Arrow error: Invalid argument error: column types must match schema types, expected Utf8View but found Utf8 at column index 0
[SQL] select
regexp_replace(col, NULL, 'c'),
regexp_replace(col, 'a', NULL),
regexp_replace(col, 'a', 'c', NULL)
from (values ('a'), ('b')) as tbl(col);
at /Users/matt/git/datafusion/datafusion/sqllogictest/test_files/regexp/regexp_replace.slt:121
```
--
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: support `Utf8View` for more args of `regexp_replace` [datafusion]
Posted by "mbutrovich (via GitHub)" <gi...@apache.org>.
mbutrovich commented on code in PR #17195:
URL: https://github.com/apache/datafusion/pull/17195#discussion_r2279783095
##########
datafusion/functions/src/regex/regexpreplace.rs:
##########
@@ -238,15 +258,17 @@ fn regex_replace_posix_groups(replacement: &str) -> String {
/// # Ok(())
/// # }
/// ```
-pub fn regexp_replace<'a, T: OffsetSizeTrait, V, B>(
- string_array: V,
- pattern_array: B,
- replacement_array: B,
- flags: Option<&ArrayRef>,
+pub fn regexp_replace_with_flags<'a, T: OffsetSizeTrait, U, V, W, X>(
+ string_array: U,
+ pattern_array: V,
Review Comment:
All 4 arrays can now be different types.
--
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: support `Utf8View` for more args of `regexp_replace` [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #17195:
URL: https://github.com/apache/datafusion/pull/17195#discussion_r2280428090
##########
datafusion/functions/src/regex/regexpreplace.rs:
##########
@@ -94,14 +98,30 @@ impl Default for RegexpReplaceFunc {
impl RegexpReplaceFunc {
pub fn new() -> Self {
- use DataType::*;
+ use TypeSignature::*;
+ use TypeSignatureClass::*;
Self {
signature: Signature::one_of(
vec![
- TypeSignature::Exact(vec![Utf8, Utf8, Utf8]),
Review Comment:
I personally recommend supporting only "all UTF8" , "all largeUtf8" and "all Utf8View"
Supporting all combinations I think is likely to generate lots of code that will make the binary bigger but not be used by anyone
##########
datafusion/functions/src/regex/regexpreplace.rs:
##########
@@ -590,44 +638,1674 @@ pub fn specialize_regexp_replace<T: OffsetSizeTrait>(
.map(|arg| arg.to_array(inferred_length))
.collect::<Result<Vec<_>>>()?;
- match args[0].data_type() {
- DataType::Utf8View => {
- let string_array = args[0].as_string_view();
- let pattern_array = args[1].as_string::<i32>();
- let replacement_array = args[2].as_string::<i32>();
- regexp_replace::<i32, _, _>(
- string_array,
- pattern_array,
- replacement_array,
- args.get(3),
- )
- }
- DataType::Utf8 => {
- let string_array = args[0].as_string::<i32>();
- let pattern_array = args[1].as_string::<i32>();
- let replacement_array = args[2].as_string::<i32>();
- regexp_replace::<i32, _, _>(
- string_array,
- pattern_array,
- replacement_array,
- args.get(3),
- )
- }
- DataType::LargeUtf8 => {
- let string_array = args[0].as_string::<i64>();
- let pattern_array = args[1].as_string::<i64>();
- let replacement_array = args[2].as_string::<i64>();
- regexp_replace::<i64, _, _>(
- string_array,
- pattern_array,
- replacement_array,
- args.get(3),
- )
+ if args.get(3).is_none() {
Review Comment:
Each of the branches in this function generates its own copy of the function (so this is generating many many different function copies
I think it makes sense to create versions where each of the parameters is the same, like
```
(DataType::Utf8View, DataType::Utf8View, DataType::Utf8View)
(DataType::Utf8, DataType::Utf8, DataType::Utf8)
(DataType::LargeUtf8, DataType::LargeUtf8, DataType::LargeUtf8)
```
I don't know how often anyone would need the other combinations 🤔
--
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: support `Utf8View` for more args of `regexp_replace` [datafusion]
Posted by "mbutrovich (via GitHub)" <gi...@apache.org>.
mbutrovich commented on code in PR #17195:
URL: https://github.com/apache/datafusion/pull/17195#discussion_r2282923454
##########
datafusion/functions/src/regex/regexpreplace.rs:
##########
@@ -590,44 +638,1674 @@ pub fn specialize_regexp_replace<T: OffsetSizeTrait>(
.map(|arg| arg.to_array(inferred_length))
.collect::<Result<Vec<_>>>()?;
- match args[0].data_type() {
- DataType::Utf8View => {
- let string_array = args[0].as_string_view();
- let pattern_array = args[1].as_string::<i32>();
- let replacement_array = args[2].as_string::<i32>();
- regexp_replace::<i32, _, _>(
- string_array,
- pattern_array,
- replacement_array,
- args.get(3),
- )
- }
- DataType::Utf8 => {
- let string_array = args[0].as_string::<i32>();
- let pattern_array = args[1].as_string::<i32>();
- let replacement_array = args[2].as_string::<i32>();
- regexp_replace::<i32, _, _>(
- string_array,
- pattern_array,
- replacement_array,
- args.get(3),
- )
- }
- DataType::LargeUtf8 => {
- let string_array = args[0].as_string::<i64>();
- let pattern_array = args[1].as_string::<i64>();
- let replacement_array = args[2].as_string::<i64>();
- regexp_replace::<i64, _, _>(
- string_array,
- pattern_array,
- replacement_array,
- args.get(3),
- )
+ if args.get(3).is_none() {
Review Comment:
For the final query in regexp_replace.slt...
```
query TTT
select
regexp_replace(col, NULL, 'c'),
regexp_replace(col, 'a', NULL),
regexp_replace(col, 'a', 'c', NULL)
from (values ('a'), ('b')) as tbl(col);
```
...DataFusion's planner calls the `return_type` method with this schema:
```
[Utf8, Utf8View, Utf8]
[Utf8, Utf8, Utf8View]
[Utf8, Utf8, Utf8, Utf8View]
```
It seems like the literal NULL is a `Utf8View`, but the other columns are `Utf8`. Maybe if I tweak the signature to have fewer options the planner will introduce the necessary casts.
I'm happy to reduce the supported types to cut down on generated code, but want to make sure we cover reasonable combinations.
--
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: better Utf8View support for `regexp_replace`, signature cleanup [datafusion]
Posted by "mbutrovich (via GitHub)" <gi...@apache.org>.
mbutrovich commented on code in PR #17195:
URL: https://github.com/apache/datafusion/pull/17195#discussion_r2277379010
##########
datafusion/functions/src/regex/regexpreplace.rs:
##########
@@ -650,8 +665,8 @@ mod tests {
vec!["afooc", "acd", "afoocd1234567890123", "123456789012afooc"];
let values = <$T>::from(values);
- let patterns = StringArray::from(patterns);
- let replacements = StringArray::from(replacement);
+ let patterns = <$U>::from(patterns);
+ let replacements = <$U>::from(replacement);
Review Comment:
I change all of the other arg string types at once since they hit the same code path (`fetch_string_arg`) so I don't think we need to test them independently.
--
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: better Utf8View support for `regexp_replace`, signature cleanup [datafusion]
Posted by "mbutrovich (via GitHub)" <gi...@apache.org>.
mbutrovich commented on code in PR #17195:
URL: https://github.com/apache/datafusion/pull/17195#discussion_r2277380381
##########
datafusion/functions/src/regex/regexpreplace.rs:
##########
@@ -125,13 +121,13 @@ impl ScalarUDFImpl for RegexpReplaceFunc {
fn return_type(&self, arg_types: &[DataType]) -> Result<DataType> {
use DataType::*;
Ok(match &arg_types[0] {
- LargeUtf8 | LargeBinary => LargeUtf8,
- Utf8 | Binary => Utf8,
- Utf8View | BinaryView => Utf8View,
+ LargeUtf8 => LargeUtf8,
Review Comment:
`Binary` and `LargeBinary` weren't in the signature and have no logic to support them.
--
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: support `Utf8View` for more args of `regexp_replace` [datafusion]
Posted by "mbutrovich (via GitHub)" <gi...@apache.org>.
mbutrovich merged PR #17195:
URL: https://github.com/apache/datafusion/pull/17195
--
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: better Utf8View support for `regexp_replace`, signature cleanup [datafusion]
Posted by "mbutrovich (via GitHub)" <gi...@apache.org>.
mbutrovich commented on code in PR #17195:
URL: https://github.com/apache/datafusion/pull/17195#discussion_r2277380381
##########
datafusion/functions/src/regex/regexpreplace.rs:
##########
@@ -125,13 +121,13 @@ impl ScalarUDFImpl for RegexpReplaceFunc {
fn return_type(&self, arg_types: &[DataType]) -> Result<DataType> {
use DataType::*;
Ok(match &arg_types[0] {
- LargeUtf8 | LargeBinary => LargeUtf8,
- Utf8 | Binary => Utf8,
- Utf8View | BinaryView => Utf8View,
+ LargeUtf8 => LargeUtf8,
Review Comment:
`Binary` and `LargeBinary` weren't even in the signature and have no logic to support them.
--
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: better Utf8View support for `regexp_replace`, signature cleanup [datafusion]
Posted by "mbutrovich (via GitHub)" <gi...@apache.org>.
mbutrovich commented on code in PR #17195:
URL: https://github.com/apache/datafusion/pull/17195#discussion_r2277380381
##########
datafusion/functions/src/regex/regexpreplace.rs:
##########
@@ -125,13 +121,13 @@ impl ScalarUDFImpl for RegexpReplaceFunc {
fn return_type(&self, arg_types: &[DataType]) -> Result<DataType> {
use DataType::*;
Ok(match &arg_types[0] {
- LargeUtf8 | LargeBinary => LargeUtf8,
- Utf8 | Binary => Utf8,
- Utf8View | BinaryView => Utf8View,
+ LargeUtf8 => LargeUtf8,
Review Comment:
`Binary` and `LargeBinary` weren't even in the signature.
--
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: support `Utf8View` for more args of `regexp_replace` [datafusion]
Posted by "mbutrovich (via GitHub)" <gi...@apache.org>.
mbutrovich commented on code in PR #17195:
URL: https://github.com/apache/datafusion/pull/17195#discussion_r2282938748
##########
datafusion/functions/src/regex/regexpreplace.rs:
##########
@@ -94,14 +98,30 @@ impl Default for RegexpReplaceFunc {
impl RegexpReplaceFunc {
pub fn new() -> Self {
- use DataType::*;
+ use TypeSignature::*;
+ use TypeSignatureClass::*;
Self {
signature: Signature::one_of(
vec![
- TypeSignature::Exact(vec![Utf8, Utf8, Utf8]),
Review Comment:
Yup binary coercion does not work with that signature:
```
External error: 1 errors in file /Users/matt/git/datafusion/datafusion/sqllogictest/test_files/binary.slt
1. query failed: DataFusion error: Error during planning: Internal error: Function 'regexp_replace' failed to match any signature, errors: Error during planning: Function 'regexp_replace' expects NativeType::String but received NativeType::Binary,Error during planning: Function 'regexp_replace' expects 4 arguments but received 3.
This issue was likely caused by a bug in DataFusion's code. Please help us to resolve this by filing a bug report in our issue tracker: https://github.com/apache/datafusion/issues No function matches the given name and argument types 'regexp_replace(Binary, Utf8, Utf8)'. You might need to add explicit type casts.
Candidate functions:
regexp_replace(String(3))
regexp_replace(String(4))
[SQL] SELECT
cast(binary as varchar) as str,
regexp_replace(binary, 'F', 'f') as binary_replaced,
cast(largebinary as varchar) as large_str,
regexp_replace(largebinary, 'F', 'f') as large_binary_replaced
from t;
at /Users/matt/git/datafusion/datafusion/sqllogictest/test_files/binary.slt:302
```
If I put it back to coercible then we need to put codepaths to handle all the wonky combinations people might come up with.
--
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: support `Utf8View` for more args of `regexp_replace` [datafusion]
Posted by "mbutrovich (via GitHub)" <gi...@apache.org>.
mbutrovich commented on code in PR #17195:
URL: https://github.com/apache/datafusion/pull/17195#discussion_r2282955561
##########
datafusion/functions/src/regex/regexpreplace.rs:
##########
@@ -94,14 +98,30 @@ impl Default for RegexpReplaceFunc {
impl RegexpReplaceFunc {
pub fn new() -> Self {
- use DataType::*;
+ use TypeSignature::*;
+ use TypeSignatureClass::*;
Self {
signature: Signature::one_of(
vec![
- TypeSignature::Exact(vec![Utf8, Utf8, Utf8]),
Review Comment:
Actually, if I reorder them into the precedence that I want...
```
signature: Signature::one_of(
vec![
Exact(vec![Utf8View, Utf8View, Utf8View]),
Exact(vec![LargeUtf8, LargeUtf8, LargeUtf8]),
Exact(vec![Utf8, Utf8, Utf8]),
Exact(vec![Utf8View, Utf8View, Utf8View, Utf8View]),
Exact(vec![LargeUtf8, LargeUtf8, LargeUtf8, LargeUtf8]),
Exact(vec![Utf8, Utf8, Utf8, Utf8]),
],
Volatility::Immutable,
),
```
I get...
```
External error: 1 errors in file /Users/matt/git/datafusion/datafusion/sqllogictest/test_files/string/string_view.slt
1. query result mismatch:
[SQL] EXPLAIN SELECT
REGEXP_REPLACE(column1_utf8view, '^https?://(?:www\.)?([^/]+)/.*$', '\1') AS k
FROM test;
[Diff] (-expected|+actual)
logical_plan
- 01)Projection: regexp_replace(test.column1_utf8view, Utf8("^https?://(?:www\.)?([^/]+)/.*$"), Utf8("\1")) AS k
+ 01)Projection: regexp_replace(test.column1_utf8view, Utf8View("^https?://(?:www\.)?([^/]+)/.*$"), Utf8View("\1")) AS k
02)--TableScan: test projection=[column1_utf8view]
at /Users/matt/git/datafusion/datafusion/sqllogictest/test_files/string/string_view.slt:801
```
Which might be okay? Casting the literals to `Utf8View` is pretty harmless.
--
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: support `Utf8View` for more args of `regexp_replace` [datafusion]
Posted by "mbutrovich (via GitHub)" <gi...@apache.org>.
mbutrovich commented on code in PR #17195:
URL: https://github.com/apache/datafusion/pull/17195#discussion_r2282929076
##########
datafusion/functions/src/regex/regexpreplace.rs:
##########
@@ -94,14 +98,30 @@ impl Default for RegexpReplaceFunc {
impl RegexpReplaceFunc {
pub fn new() -> Self {
- use DataType::*;
+ use TypeSignature::*;
+ use TypeSignatureClass::*;
Self {
signature: Signature::one_of(
vec![
- TypeSignature::Exact(vec![Utf8, Utf8, Utf8]),
Review Comment:
I think that's equivalent to a signature of
```
signature: Signature::one_of(
vec![
String(3),
String(4)
],
Volatility::Immutable,
),
```
IIRC I ran into issues with that (with binary types), but I agree it is simpler. I'll revert to that and see how it goes.
--
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: support `Utf8View` for more args of `regexp_replace` [datafusion]
Posted by "mbutrovich (via GitHub)" <gi...@apache.org>.
mbutrovich commented on PR #17195:
URL: https://github.com/apache/datafusion/pull/17195#issuecomment-3225676497
I haven't forgotten about this. I need to generate a new .slt file for the relevant tests.
--
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: support `Utf8View` for more args of `regexp_replace` [datafusion]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on code in PR #17195:
URL: https://github.com/apache/datafusion/pull/17195#discussion_r2348023637
##########
datafusion/functions/src/regex/regexpreplace.rs:
##########
@@ -590,44 +613,109 @@ pub fn specialize_regexp_replace<T: OffsetSizeTrait>(
.map(|arg| arg.to_array(inferred_length))
.collect::<Result<Vec<_>>>()?;
- match args[0].data_type() {
- DataType::Utf8View => {
- let string_array = args[0].as_string_view();
- let pattern_array = args[1].as_string::<i32>();
- let replacement_array = args[2].as_string::<i32>();
- regexp_replace::<i32, _, _>(
- string_array,
- pattern_array,
- replacement_array,
- args.get(3),
- )
- }
- DataType::Utf8 => {
- let string_array = args[0].as_string::<i32>();
- let pattern_array = args[1].as_string::<i32>();
- let replacement_array = args[2].as_string::<i32>();
- regexp_replace::<i32, _, _>(
- string_array,
- pattern_array,
- replacement_array,
- args.get(3),
- )
- }
- DataType::LargeUtf8 => {
- let string_array = args[0].as_string::<i64>();
- let pattern_array = args[1].as_string::<i64>();
- let replacement_array = args[2].as_string::<i64>();
- regexp_replace::<i64, _, _>(
- string_array,
- pattern_array,
- replacement_array,
- args.get(3),
- )
+ if args.get(3).is_none() {
Review Comment:
We could fold the if into the match itself like so:
```rust
match (
args[0].data_type(),
args[1].data_type(),
args[2].data_type(),
args.get(3).map(|a| a.data_type()),
) {
(
DataType::Utf8,
DataType::Utf8,
DataType::Utf8,
Some(DataType::Utf8) | None,
) => {
let string_array = args[0].as_string::<i32>();
let pattern_array = args[1].as_string::<i32>();
let replacement_array = args[2].as_string::<i32>();
let flags_array = args.get(3).map(|a| a.as_string::<i32>());
regexp_replace::<i32, _>(
string_array,
pattern_array,
replacement_array,
flags_array,
)
}
(
DataType::Utf8View,
DataType::Utf8View,
DataType::Utf8View,
Some(DataType::Utf8View) | None,
) => {
let string_array = args[0].as_string_view();
let pattern_array = args[1].as_string_view();
let replacement_array = args[2].as_string_view();
let flags_array = args.get(3).map(|a| a.as_string_view());
regexp_replace::<i32, _>(
string_array,
pattern_array,
replacement_array,
flags_array,
)
}
(
DataType::LargeUtf8,
DataType::LargeUtf8,
DataType::LargeUtf8,
Some(DataType::LargeUtf8) | None,
) => {
let string_array = args[0].as_string::<i64>();
let pattern_array = args[1].as_string::<i64>();
let replacement_array = args[2].as_string::<i64>();
let flags_array = args.get(3).map(|a| a.as_string::<i64>());
regexp_replace::<i64, _>(
string_array,
pattern_array,
replacement_array,
flags_array,
)
}
other => {
exec_err!(
"Unsupported data type {other:?} for function regex_replace"
)
}
```
Removes the duplication across the if arms
##########
datafusion/functions/src/regex/regexpreplace.rs:
##########
@@ -94,14 +98,30 @@ impl Default for RegexpReplaceFunc {
impl RegexpReplaceFunc {
pub fn new() -> Self {
- use DataType::*;
+ use TypeSignature::*;
+ use TypeSignatureClass::*;
Self {
signature: Signature::one_of(
vec![
- TypeSignature::Exact(vec![Utf8, Utf8, Utf8]),
Review Comment:
I think this is an option too?
```rust
Uniform(3, vec![Utf8View, LargeUtf8, Utf8]),
Uniform(4, vec![Utf8View, LargeUtf8, Utf8]),
```
I tested it on a checked out copy of this PR and the slt tests passed
--
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: support `Utf8View` for more args of `regexp_replace` [datafusion]
Posted by "mbutrovich (via GitHub)" <gi...@apache.org>.
mbutrovich commented on PR #17195:
URL: https://github.com/apache/datafusion/pull/17195#issuecomment-3192718767
Tested clickbench q28 which has `regexp_replace` and no regression on the fast-path:
main
```
SELECT REGEXP_REPLACE("Referer", '^https?://(?:www\.)?([^/]+)/.*$', '\1') AS k, AVG(length("Referer")) AS l, COUNT(*) AS c, MIN("Referer") FROM hits WHERE "Referer" <> '' GROUP BY k HAVING COUNT(*) > 100000 ORDER BY l DESC LIMIT 25;
Query 28 iteration 0 took 5757.7 ms and returned 25 rows
Query 28 iteration 1 took 5735.3 ms and returned 25 rows
Query 28 iteration 2 took 5750.6 ms and returned 25 rows
Query 28 iteration 3 took 5735.3 ms and returned 25 rows
Query 28 iteration 4 took 5782.7 ms and returned 25 rows
Query 28 avg time: 5752.31 ms
```
PR
```
SELECT REGEXP_REPLACE("Referer", '^https?://(?:www\.)?([^/]+)/.*$', '\1') AS k, AVG(length("Referer")) AS l, COUNT(*) AS c, MIN("Referer") FROM hits WHERE "Referer" <> '' GROUP BY k HAVING COUNT(*) > 100000 ORDER BY l DESC LIMIT 25;
Query 28 iteration 0 took 5717.5 ms and returned 25 rows
Query 28 iteration 1 took 5735.0 ms and returned 25 rows
Query 28 iteration 2 took 5750.9 ms and returned 25 rows
Query 28 iteration 3 took 5751.8 ms and returned 25 rows
Query 28 iteration 4 took 5739.1 ms and returned 25 rows
Query 28 avg time: 5738.83 ms
```
--
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: support `Utf8View` for more args of `regexp_replace` [datafusion]
Posted by "mbutrovich (via GitHub)" <gi...@apache.org>.
mbutrovich commented on code in PR #17195:
URL: https://github.com/apache/datafusion/pull/17195#discussion_r2282955561
##########
datafusion/functions/src/regex/regexpreplace.rs:
##########
@@ -94,14 +98,30 @@ impl Default for RegexpReplaceFunc {
impl RegexpReplaceFunc {
pub fn new() -> Self {
- use DataType::*;
+ use TypeSignature::*;
+ use TypeSignatureClass::*;
Self {
signature: Signature::one_of(
vec![
- TypeSignature::Exact(vec![Utf8, Utf8, Utf8]),
Review Comment:
Actually, if I reorder them into the precedence that I want...
```
signature: Signature::one_of(
vec![
Exact(vec![Utf8View, Utf8View, Utf8View]),
Exact(vec![LargeUtf8, LargeUtf8, LargeUtf8]),
Exact(vec![Utf8, Utf8, Utf8]),
Exact(vec![Utf8View, Utf8View, Utf8View, Utf8View]),
Exact(vec![LargeUtf8, LargeUtf8, LargeUtf8, LargeUtf8]),
Exact(vec![Utf8, Utf8, Utf8, Utf8]),
],
Volatility::Immutable,
),
```
I get...
```
External error: 1 errors in file /Users/matt/git/datafusion/datafusion/sqllogictest/test_files/string/string_view.slt
1. query result mismatch:
[SQL] EXPLAIN SELECT
REGEXP_REPLACE(column1_utf8view, '^https?://(?:www\.)?([^/]+)/.*$', '\1') AS k
FROM test;
[Diff] (-expected|+actual)
logical_plan
- 01)Projection: regexp_replace(test.column1_utf8view, Utf8("^https?://(?:www\.)?([^/]+)/.*$"), Utf8("\1")) AS k
+ 01)Projection: regexp_replace(test.column1_utf8view, Utf8View("^https?://(?:www\.)?([^/]+)/.*$"), Utf8View("\1")) AS k
02)--TableScan: test projection=[column1_utf8view]
at /Users/matt/git/datafusion/datafusion/sqllogictest/test_files/string/string_view.slt:801
```
Which might be okay? Defining the literals as `Utf8View` is pretty harmless.
--
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: support `Utf8View` for more args of `regexp_replace` [datafusion]
Posted by "mbutrovich (via GitHub)" <gi...@apache.org>.
mbutrovich commented on PR #17195:
URL: https://github.com/apache/datafusion/pull/17195#issuecomment-3192570690
I'm not proud of the readability of the change, but at least there doesn't seem to be a meaningful performance regression:
main
```
regexp_replace_1000 time: [1.5948 ms 1.5958 ms 1.5967 ms]
regexp_replace_1000 utf8view
time: [1.5956 ms 1.5966 ms 1.5976 ms]
```
PR
```
regexp_replace_1000 time: [1.5982 ms 1.5991 ms 1.6000 ms]
regexp_replace_1000 utf8view
time: [1.6023 ms 1.6038 ms 1.6056 ms]
```
--
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: support `Utf8View` for more args of `regexp_replace` [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #17195:
URL: https://github.com/apache/datafusion/pull/17195#issuecomment-3207027997
@mbutrovich -- thank you for this PR.
I still can't help but feel there is a mismatch between what this PR is doing (trying expand the types of string arguments this function supports) and what you are actually seeing in Comet.
Can you explain what is happening in comet? For example, is regexp_replace being called with `regexp_replace(UTF8View, Utf8View, Utf8View)`
I ask because a mismatch between the actual argument types, and the types a function accepts can be fixed in at least 2 ways:
1. Expand the types a function supports (this PR)
2. Cast the arguments to some combination that the function supports (via coercion).
Typically coercion is done in the LogicalPlan, and I know that Comet does not use LogicalPlans. However, @adriangb and @kosiew have been working on a version of this for `PhysicalExprs`, in the [PhysicalExprAdapter](https://docs.rs/datafusion/latest/datafusion/physical_expr/schema_rewriter/trait.PhysicalExprAdapter.html)
I also reviewed the documentation on types and type signatures and I think there is some ambiguity about what a function "supporting argument types" means. I will make a PR to improve the documentation in this area
--
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: support `Utf8View` for more args of `regexp_replace` [datafusion]
Posted by "mbutrovich (via GitHub)" <gi...@apache.org>.
mbutrovich commented on PR #17195:
URL: https://github.com/apache/datafusion/pull/17195#issuecomment-3271252630
> Can you explain what is happening in comet? For example, is regexp_replace being called with `regexp_replace(UTF8View, Utf8View, Utf8View)`
In my experimental branch adding `StringView` support to Comet, we need a way to represent string literals during serialization from the Spark side to DataFusion. Currently all string literals come over as `Utf8` and that just works. However, with `Utf8View` columns coming out of the Parquet reader, Arrow complains about not being able to evaluate filter expressions with mismatched types. I changed all string literals to be `Utf8View`, which underneath doesn't really change anything underneath for single `ScalarValue`s. Now, however, I have problems with functions like `regexp_replace` which expect literals to be `Utf8`. Since Comet does not use DataFusion's front-end, we don't get the cast operations inserted into the plan that the signature logic is designed for.
I am increasingly of the mind that Comet needs to start doing some passes over the physical plan, and type coercion like this might be one reason.
I think this PR is good to go, but also am okay if we think it's needless complexity.
--
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: support `Utf8View` for more args of `regexp_replace` [datafusion]
Posted by "mbutrovich (via GitHub)" <gi...@apache.org>.
mbutrovich commented on code in PR #17195:
URL: https://github.com/apache/datafusion/pull/17195#discussion_r2279783095
##########
datafusion/functions/src/regex/regexpreplace.rs:
##########
@@ -238,15 +258,17 @@ fn regex_replace_posix_groups(replacement: &str) -> String {
/// # Ok(())
/// # }
/// ```
-pub fn regexp_replace<'a, T: OffsetSizeTrait, V, B>(
- string_array: V,
- pattern_array: B,
- replacement_array: B,
- flags: Option<&ArrayRef>,
+pub fn regexp_replace_with_flags<'a, T: OffsetSizeTrait, U, V, W, X>(
+ string_array: U,
+ pattern_array: V,
Review Comment:
All 4 arrays can now be different types.
--
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: support `Utf8View` for more args of `regexp_replace` [datafusion]
Posted by "mbutrovich (via GitHub)" <gi...@apache.org>.
mbutrovich commented on code in PR #17195:
URL: https://github.com/apache/datafusion/pull/17195#discussion_r2282951345
##########
datafusion/functions/src/regex/regexpreplace.rs:
##########
@@ -94,14 +98,30 @@ impl Default for RegexpReplaceFunc {
impl RegexpReplaceFunc {
pub fn new() -> Self {
- use DataType::*;
+ use TypeSignature::*;
+ use TypeSignatureClass::*;
Self {
signature: Signature::one_of(
vec![
- TypeSignature::Exact(vec![Utf8, Utf8, Utf8]),
Review Comment:
If I do the signature as...
```
signature: Signature::one_of(
vec![
Exact(vec![Utf8, Utf8, Utf8]),
Exact(vec![LargeUtf8, LargeUtf8, LargeUtf8]),
Exact(vec![Utf8View, Utf8View, Utf8View]),
Exact(vec![Utf8, Utf8, Utf8, Utf8]),
Exact(vec![LargeUtf8, LargeUtf8, LargeUtf8, LargeUtf8]),
Exact(vec![Utf8View, Utf8View, Utf8View, Utf8View]),
],
Volatility::Immutable,
),
```
...that generates this test failure:
```
External error: 1 errors in file /Users/matt/git/datafusion/datafusion/sqllogictest/test_files/string/string_view.slt
1. query result mismatch:
[SQL] EXPLAIN SELECT
REGEXP_REPLACE(column1_utf8view, '^https?://(?:www\.)?([^/]+)/.*$', '\1') AS k
FROM test;
[Diff] (-expected|+actual)
logical_plan
- 01)Projection: regexp_replace(test.column1_utf8view, Utf8("^https?://(?:www\.)?([^/]+)/.*$"), Utf8("\1")) AS k
+ 01)Projection: regexp_replace(CAST(test.column1_utf8view AS Utf8), Utf8("^https?://(?:www\.)?([^/]+)/.*$"), Utf8("\1")) AS k
02)--TableScan: test projection=[column1_utf8view]
at /Users/matt/git/datafusion/datafusion/sqllogictest/test_files/string/string_view.slt:801
```
which implies that a `Utf8View` column would get cast to `Utf8` with other `Utf8` literals as args. That's not good.
The signature with `String(3)` or `String(4)` seems like the best option if I can understand why binary coercion does not work with that.
--
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: support `Utf8View` for more args of `regexp_replace` [datafusion]
Posted by "mbutrovich (via GitHub)" <gi...@apache.org>.
mbutrovich commented on code in PR #17195:
URL: https://github.com/apache/datafusion/pull/17195#discussion_r2282929076
##########
datafusion/functions/src/regex/regexpreplace.rs:
##########
@@ -94,14 +98,30 @@ impl Default for RegexpReplaceFunc {
impl RegexpReplaceFunc {
pub fn new() -> Self {
- use DataType::*;
+ use TypeSignature::*;
+ use TypeSignatureClass::*;
Self {
signature: Signature::one_of(
vec![
- TypeSignature::Exact(vec![Utf8, Utf8, Utf8]),
Review Comment:
I think that's equivalent to a signature of ```
signature: Signature::one_of(
vec![
String(3),
String(4)
],
Volatility::Immutable,
),
```
IIRC I ran into issues with that, but I agree it is simpler.
##########
datafusion/functions/src/regex/regexpreplace.rs:
##########
@@ -94,14 +98,30 @@ impl Default for RegexpReplaceFunc {
impl RegexpReplaceFunc {
pub fn new() -> Self {
- use DataType::*;
+ use TypeSignature::*;
+ use TypeSignatureClass::*;
Self {
signature: Signature::one_of(
vec![
- TypeSignature::Exact(vec![Utf8, Utf8, Utf8]),
Review Comment:
I think that's equivalent to a signature of
```
signature: Signature::one_of(
vec![
String(3),
String(4)
],
Volatility::Immutable,
),
```
IIRC I ran into issues with that, but I agree it is simpler.
--
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: better Utf8View support for `regexp_replace`, signature cleanup [datafusion]
Posted by "mbutrovich (via GitHub)" <gi...@apache.org>.
mbutrovich commented on code in PR #17195:
URL: https://github.com/apache/datafusion/pull/17195#discussion_r2277377451
##########
datafusion/functions/src/regex/regexpreplace.rs:
##########
@@ -398,12 +394,37 @@ fn _regexp_replace_early_abort<T: ArrayAccessor>(
/// Note: If the array is empty or the first argument is null,
/// then calls the given early abort function.
macro_rules! fetch_string_arg {
- ($ARG:expr, $NAME:expr, $T:ident, $EARLY_ABORT:ident, $ARRAY_SIZE:expr) => {{
- let array = as_generic_string_array::<$T>($ARG)?;
- if array.len() == 0 || array.is_null(0) {
- return $EARLY_ABORT(array, $ARRAY_SIZE);
- } else {
- array.value(0)
+ ($ARG:expr, $NAME:expr, $EARLY_ABORT:ident, $ARRAY_SIZE:expr) => {{
Review Comment:
I'm not sure how the `T` argument worked before, since it's based on whether `arg[0]` was `Utf8` or `LargeUtf8` and shouldn't affect the other argument arrays.
--
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: support Utf8View for more args of `regexp_replace` [datafusion]
Posted by "mbutrovich (via GitHub)" <gi...@apache.org>.
mbutrovich commented on code in PR #17195:
URL: https://github.com/apache/datafusion/pull/17195#discussion_r2277380381
##########
datafusion/functions/src/regex/regexpreplace.rs:
##########
@@ -125,13 +121,13 @@ impl ScalarUDFImpl for RegexpReplaceFunc {
fn return_type(&self, arg_types: &[DataType]) -> Result<DataType> {
use DataType::*;
Ok(match &arg_types[0] {
- LargeUtf8 | LargeBinary => LargeUtf8,
- Utf8 | Binary => Utf8,
- Utf8View | BinaryView => Utf8View,
+ LargeUtf8 => LargeUtf8,
Review Comment:
`Binary` and `LargeBinary` weren't in the signature and have no logic to support them.
--
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: support `Utf8View` for more args of `regexp_replace` [datafusion]
Posted by "mbutrovich (via GitHub)" <gi...@apache.org>.
mbutrovich commented on PR #17195:
URL: https://github.com/apache/datafusion/pull/17195#issuecomment-3329028235
Thanks for the reviews and helpful feedback @Jefffrey and @alamb! I'm hopeful that this will get Comet closer to fully supporting `Utf8View`.
--
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