You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "devanbenz (via GitHub)" <gi...@apache.org> on 2024/08/29 02:01:57 UTC
[PR] Update the CONCAT scalar function to support Utf8View [datafusion]
devanbenz opened a new pull request, #12224:
URL: https://github.com/apache/datafusion/pull/12224
## 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.
-->
## 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.
-->
## 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)?
-->
## 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.
-->
--
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] Update the CONCAT scalar function to support Utf8View [datafusion]
Posted by "devanbenz (via GitHub)" <gi...@apache.org>.
devanbenz commented on code in PR #12224:
URL: https://github.com/apache/datafusion/pull/12224#discussion_r1739513063
##########
datafusion/functions/src/string/concat.rs:
##########
@@ -64,13 +66,19 @@ impl ScalarUDFImpl for ConcatFunc {
&self.signature
}
- fn return_type(&self, _arg_types: &[DataType]) -> Result<DataType> {
- Ok(Utf8)
+ fn return_type(&self, arg_types: &[DataType]) -> Result<DataType> {
+ use DataType::*;
+ Ok(match &arg_types[0] {
+ Utf8View => Utf8View,
+ LargeUtf8 => LargeUtf8,
+ _ => Utf8,
+ })
Review Comment:
@findepi when you say binary concat operator are you talking about `||` as an operator?
--
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] Update the CONCAT scalar function to support Utf8View [datafusion]
Posted by "findepi (via GitHub)" <gi...@apache.org>.
findepi commented on code in PR #12224:
URL: https://github.com/apache/datafusion/pull/12224#discussion_r1739363646
##########
datafusion/functions/src/string/concat.rs:
##########
@@ -64,13 +66,19 @@ impl ScalarUDFImpl for ConcatFunc {
&self.signature
}
- fn return_type(&self, _arg_types: &[DataType]) -> Result<DataType> {
- Ok(Utf8)
+ fn return_type(&self, arg_types: &[DataType]) -> Result<DataType> {
+ use DataType::*;
+ Ok(match &arg_types[0] {
+ Utf8View => Utf8View,
+ LargeUtf8 => LargeUtf8,
+ _ => Utf8,
+ })
Review Comment:
i don't know the exact rules for how we handle LargeUtf8.
i focused on the string view portion of the PR. from "string views perspective", LargeUtf8 is non-issue, so IMO it's fine not to change the return type with respect to LargeUtf8 in this PR. but i agree that we probably should return LargeUtf8 when any input is LargeUtf8 (pr what exactly the logic should be).
in fact, what does the binary concat operator do?
--
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] Update the CONCAT scalar function to support Utf8View [datafusion]
Posted by "devanbenz (via GitHub)" <gi...@apache.org>.
devanbenz commented on code in PR #12224:
URL: https://github.com/apache/datafusion/pull/12224#discussion_r1737429136
##########
datafusion/functions/src/string/concat.rs:
##########
@@ -46,7 +45,7 @@ impl ConcatFunc {
pub fn new() -> Self {
use DataType::*;
Self {
- signature: Signature::variadic(vec![Utf8], Volatility::Immutable),
+ signature: Signature::variadic(vec![Utf8, Utf8View], Volatility::Immutable),
Review Comment:
Actually it's erroring on cast to generic string array. I need to modify 😆 whoops still not fixed
--
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] Update the CONCAT scalar function to support Utf8View [datafusion]
Posted by "findepi (via GitHub)" <gi...@apache.org>.
findepi commented on code in PR #12224:
URL: https://github.com/apache/datafusion/pull/12224#discussion_r1738528813
##########
datafusion/functions/src/string/concat.rs:
##########
@@ -64,13 +66,19 @@ impl ScalarUDFImpl for ConcatFunc {
&self.signature
}
- fn return_type(&self, _arg_types: &[DataType]) -> Result<DataType> {
- Ok(Utf8)
+ fn return_type(&self, arg_types: &[DataType]) -> Result<DataType> {
+ use DataType::*;
+ Ok(match &arg_types[0] {
+ Utf8View => Utf8View,
+ LargeUtf8 => LargeUtf8,
+ _ => Utf8,
+ })
Review Comment:
yes, that's my intuition
except that i would keep building StringArray and just declare return type as Utf8 always
from the issue https://github.com/apache/datafusion/issues/11836
> Currently, a call to CONCAT with a Utf8View datatypes induces a cast. After the change that fixes this issue, it should not.
this is about _inputs_ to the function, not the return type
Side note:
String view could be an interesting return type if we wanted to optimize for single non-null string view input and let it pass-through; but the code doesn't do this today, not sure it's worth implementing for this edge case and it should be independent of arguments order, ie not tied to the first input's type.
end of side note.
--
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] Update the CONCAT scalar function to support Utf8View [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #12224:
URL: https://github.com/apache/datafusion/pull/12224#issuecomment-2327354406
Thanks again @devanbenz
--
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] Update the CONCAT scalar function to support Utf8View [datafusion]
Posted by "findepi (via GitHub)" <gi...@apache.org>.
findepi commented on code in PR #12224:
URL: https://github.com/apache/datafusion/pull/12224#discussion_r1737223415
##########
datafusion/functions/src/string/concat.rs:
##########
@@ -64,13 +66,19 @@ impl ScalarUDFImpl for ConcatFunc {
&self.signature
}
- fn return_type(&self, _arg_types: &[DataType]) -> Result<DataType> {
- Ok(Utf8)
+ fn return_type(&self, arg_types: &[DataType]) -> Result<DataType> {
+ use DataType::*;
+ Ok(match &arg_types[0] {
+ Utf8View => Utf8View,
+ LargeUtf8 => LargeUtf8,
+ _ => Utf8,
+ })
Review Comment:
the logic seems to assume all arguments are of the same type?
also, why not always return `Utf8`?
the code performing actual concatenation seems to be always the same.
##########
datafusion/functions/src/string/concat.rs:
##########
@@ -64,13 +66,19 @@ impl ScalarUDFImpl for ConcatFunc {
&self.signature
}
- fn return_type(&self, _arg_types: &[DataType]) -> Result<DataType> {
- Ok(Utf8)
+ fn return_type(&self, arg_types: &[DataType]) -> Result<DataType> {
+ use DataType::*;
+ Ok(match &arg_types[0] {
+ Utf8View => Utf8View,
+ LargeUtf8 => LargeUtf8,
+ _ => Utf8,
+ })
}
/// Concatenates the text representations of all the arguments. NULL arguments are ignored.
/// concat('abcde', 2, NULL, 22) = 'abcde222'
fn invoke(&self, args: &[ColumnarValue]) -> Result<ColumnarValue> {
+ let args_datatype = args[0].data_type();
Review Comment:
- can the `args` slice be empty?
- args_datatype denotes first argument's data type, so let's call the variable to reflect 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] Update the CONCAT scalar function to support Utf8View [datafusion]
Posted by "devanbenz (via GitHub)" <gi...@apache.org>.
devanbenz commented on code in PR #12224:
URL: https://github.com/apache/datafusion/pull/12224#discussion_r1737187052
##########
datafusion/functions/src/string/concat.rs:
##########
@@ -46,7 +45,7 @@ impl ConcatFunc {
pub fn new() -> Self {
use DataType::*;
Self {
- signature: Signature::variadic(vec![Utf8], Volatility::Immutable),
+ signature: Signature::variadic(vec![Utf8, Utf8View], Volatility::Immutable),
Review Comment:
Correction** I fixed it now it was really quick resolution lol
--
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] Update the CONCAT scalar function to support Utf8View [datafusion]
Posted by "findepi (via GitHub)" <gi...@apache.org>.
findepi commented on code in PR #12224:
URL: https://github.com/apache/datafusion/pull/12224#discussion_r1738447517
##########
datafusion/functions/src/string/concat.rs:
##########
@@ -64,13 +66,19 @@ impl ScalarUDFImpl for ConcatFunc {
&self.signature
}
- fn return_type(&self, _arg_types: &[DataType]) -> Result<DataType> {
- Ok(Utf8)
+ fn return_type(&self, arg_types: &[DataType]) -> Result<DataType> {
+ use DataType::*;
+ Ok(match &arg_types[0] {
+ Utf8View => Utf8View,
+ LargeUtf8 => LargeUtf8,
+ _ => Utf8,
+ })
Review Comment:
> I think the logic is "Whatever the first argument type is the output should be of that type
i understand this is what's implemented. but not sure why it is so.
what's the exact benefit of presenting the data as string view, if we computed the exact string anyway, and we technically don't need to represent it as a string view?
--
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] Update the CONCAT scalar function to support Utf8View [datafusion]
Posted by "devanbenz (via GitHub)" <gi...@apache.org>.
devanbenz commented on code in PR #12224:
URL: https://github.com/apache/datafusion/pull/12224#discussion_r1737202952
##########
datafusion/functions/src/string/concat.rs:
##########
@@ -46,7 +45,7 @@ impl ConcatFunc {
pub fn new() -> Self {
use DataType::*;
Self {
- signature: Signature::variadic(vec![Utf8], Volatility::Immutable),
+ signature: Signature::variadic(vec![Utf8, Utf8View], Volatility::Immutable),
Review Comment:
```
> SELECT ttt.a from ttt;
+---+
| a |
+---+
| T |
+---+
1 row(s) fetched.
Elapsed 0.009 seconds.
> EXPLAIN SELECT ttt.a from ttt;
+---------------+-----------------------------------------------+
| plan_type | plan |
+---------------+-----------------------------------------------+
| logical_plan | TableScan: ttt projection=[a] |
| physical_plan | MemoryExec: partitions=1, partition_sizes=[1] |
| | |
+---------------+-----------------------------------------------+
2 row(s) fetched.
Elapsed 0.007 seconds.
> EXPLAIN SELECT concat(ttt.a, ttt.a) from ttt;
+---------------+----------------------------------------------------------------+
| plan_type | plan |
+---------------+----------------------------------------------------------------+
| logical_plan | Projection: concat(ttt.a, ttt.a) |
| | TableScan: ttt projection=[a] |
| physical_plan | ProjectionExec: expr=[concat(a@0, a@0) as concat(ttt.a,ttt.a)] |
| | MemoryExec: partitions=1, partition_sizes=[1] |
| | |
+---------------+----------------------------------------------------------------+
2 row(s) fetched.
Elapsed 0.009 seconds.
```
--
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] Update the CONCAT scalar function to support Utf8View [datafusion]
Posted by "devanbenz (via GitHub)" <gi...@apache.org>.
devanbenz commented on code in PR #12224:
URL: https://github.com/apache/datafusion/pull/12224#discussion_r1738588202
##########
datafusion/functions/src/string/concat.rs:
##########
@@ -46,7 +45,7 @@ impl ConcatFunc {
pub fn new() -> Self {
use DataType::*;
Self {
- signature: Signature::variadic(vec![Utf8], Volatility::Immutable),
+ signature: Signature::variadic(vec![Utf8, Utf8View], Volatility::Immutable),
Review Comment:
It's resolved 👍
--
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] Update the CONCAT scalar function to support Utf8View [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #12224:
URL: https://github.com/apache/datafusion/pull/12224#discussion_r1740777076
##########
datafusion/functions/src/string/concat.rs:
##########
@@ -64,13 +66,36 @@ impl ScalarUDFImpl for ConcatFunc {
&self.signature
}
- fn return_type(&self, _arg_types: &[DataType]) -> Result<DataType> {
- Ok(Utf8)
+ fn return_type(&self, arg_types: &[DataType]) -> Result<DataType> {
+ use DataType::*;
+ let mut dt = &Utf8;
+ arg_types.iter().for_each(|data_type| {
+ if data_type == &Utf8View {
+ dt = data_type;
+ }
+ if data_type == &LargeUtf8 && dt != &Utf8View {
Review Comment:
👍
##########
datafusion/functions/src/string/common.rs:
##########
@@ -418,6 +448,148 @@ impl StringArrayBuilder {
}
}
+pub(crate) struct StringViewArrayBuilder {
+ builder: StringViewBuilder,
+ block: String
+}
+
+impl StringViewArrayBuilder {
Review Comment:
I think the original intent of `StringViewBuilder` when @JasonLi-cn added it was to have a way to append (but not complete) strings to avoid a copy and not incrementally update the null mask.
Specifically, StringBuilder in arrow https://docs.rs/arrow/latest/arrow/array/type.StringBuilder.html#method.append_value lets you append an entire string but it requires the input be contiguous bytes
`StringArrayBuilder` is that you can push bytes (via `write`) wthout copying the data.
The reason I think it feels hacky is that the API for building up single values in a StringArray or StringViewArray without a copy is not present
As we see we want to do the same thing with `Utf8View` I wonder if we should spend some time making the API eaiser to use 🤔
##########
datafusion/functions/src/string/common.rs:
##########
@@ -418,6 +448,148 @@ impl StringArrayBuilder {
}
}
+pub(crate) struct StringViewArrayBuilder {
+ builder: StringViewBuilder,
+ block: String
+}
+
+impl StringViewArrayBuilder {
Review Comment:
I filed https://github.com/apache/arrow-rs/issues/6347 to discuss adding an API like this in arrow as well
--
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] Update the CONCAT scalar function to support Utf8View [datafusion]
Posted by "devanbenz (via GitHub)" <gi...@apache.org>.
devanbenz commented on code in PR #12224:
URL: https://github.com/apache/datafusion/pull/12224#discussion_r1740156822
##########
datafusion/functions/src/string/common.rs:
##########
@@ -418,6 +448,148 @@ impl StringArrayBuilder {
}
}
+pub(crate) struct StringViewArrayBuilder {
+ builder: StringViewBuilder,
+ block: String
+}
+
+impl StringViewArrayBuilder {
Review Comment:
`StringViewArrayBuilder` works but it almost feels "hacky"
--
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] Update the CONCAT scalar function to support Utf8View [datafusion]
Posted by "tshauck (via GitHub)" <gi...@apache.org>.
tshauck commented on code in PR #12224:
URL: https://github.com/apache/datafusion/pull/12224#discussion_r1737152586
##########
datafusion/functions/src/string/concat.rs:
##########
@@ -46,7 +45,7 @@ impl ConcatFunc {
pub fn new() -> Self {
use DataType::*;
Self {
- signature: Signature::variadic(vec![Utf8], Volatility::Immutable),
+ signature: Signature::variadic(vec![Utf8, Utf8View], Volatility::Immutable),
Review Comment:
Sorry, should've been clearer, to your point, I think it works but causes a cast to utf8.
```
> CREATE TABLE ttt AS SELECT arrow_cast('T', 'LargeUtf8') AS a;
0 row(s) fetched.
Elapsed 0.012 seconds.
> SELECt ttt.a FROM ttt;
+---+
| a |
+---+
| T |
+---+
1 row(s) fetched.
Elapsed 0.007 seconds.
> EXPLAIN SELECt ttt.a FROM ttt;
+---------------+-----------------------------------------------+
| plan_type | plan |
+---------------+-----------------------------------------------+
| logical_plan | TableScan: ttt projection=[a] |
| physical_plan | MemoryExec: partitions=1, partition_sizes=[1] |
| | |
+---------------+-----------------------------------------------+
2 row(s) fetched.
Elapsed 0.007 seconds.
> EXPLAIN SELECt concat(ttt.a, ttt.a) FROM ttt;
+---------------+--------------------------------------------------------------------------------------------+
| plan_type | plan |
+---------------+--------------------------------------------------------------------------------------------+
| logical_plan | Projection: concat(__common_expr_1 AS ttt.a, __common_expr_1 AS ttt.a) |
| | Projection: CAST(ttt.a AS Utf8) AS __common_expr_1 |
| | TableScan: ttt projection=[a] |
| physical_plan | ProjectionExec: expr=[concat(__common_expr_1@0, __common_expr_1@0) as concat(ttt.a,ttt.a)] |
| | ProjectionExec: expr=[CAST(a@0 AS Utf8) as __common_expr_1] |
| | MemoryExec: partitions=1, partition_sizes=[1] |
| | |
+---------------+--------------------------------------------------------------------------------------------+
```
--
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] Update the CONCAT scalar function to support Utf8View [datafusion]
Posted by "devanbenz (via GitHub)" <gi...@apache.org>.
devanbenz commented on code in PR #12224:
URL: https://github.com/apache/datafusion/pull/12224#discussion_r1738561130
##########
datafusion/functions/src/string/concat.rs:
##########
@@ -64,13 +66,19 @@ impl ScalarUDFImpl for ConcatFunc {
&self.signature
}
- fn return_type(&self, _arg_types: &[DataType]) -> Result<DataType> {
- Ok(Utf8)
+ fn return_type(&self, arg_types: &[DataType]) -> Result<DataType> {
+ use DataType::*;
+ Ok(match &arg_types[0] {
+ Utf8View => Utf8View,
+ LargeUtf8 => LargeUtf8,
+ _ => Utf8,
+ })
Review Comment:
Great that sounds good with me I'll go ahead and adjust the code.
--
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] Update the CONCAT scalar function to support Utf8View [datafusion]
Posted by "tshauck (via GitHub)" <gi...@apache.org>.
tshauck commented on code in PR #12224:
URL: https://github.com/apache/datafusion/pull/12224#discussion_r1737113525
##########
datafusion/sqllogictest/test_files/string_view.slt:
##########
@@ -776,7 +776,7 @@ EXPLAIN SELECT
FROM test;
----
logical_plan
-01)Projection: concat(CAST(test.column1_utf8view AS Utf8), CAST(test.column2_utf8view AS Utf8)) AS c
+01)Projection: concat(test.column1_utf8view, test.column2_utf8view) AS c
Review Comment:
💪
##########
datafusion/functions/src/string/concat.rs:
##########
@@ -46,7 +45,7 @@ impl ConcatFunc {
pub fn new() -> Self {
use DataType::*;
Self {
- signature: Signature::variadic(vec![Utf8], Volatility::Immutable),
+ signature: Signature::variadic(vec![Utf8, Utf8View], Volatility::Immutable),
Review Comment:
Perhaps as a follow on PR we can expand this to also support `LargeUtf8`s... though it looks like maybe they are from the tests? Does this just need to be updated and it'll work?
--
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] Update the CONCAT scalar function to support Utf8View [datafusion]
Posted by "devanbenz (via GitHub)" <gi...@apache.org>.
devanbenz commented on code in PR #12224:
URL: https://github.com/apache/datafusion/pull/12224#discussion_r1737125586
##########
datafusion/functions/src/string/concat.rs:
##########
@@ -46,7 +45,7 @@ impl ConcatFunc {
pub fn new() -> Self {
use DataType::*;
Self {
- signature: Signature::variadic(vec![Utf8], Volatility::Immutable),
+ signature: Signature::variadic(vec![Utf8, Utf8View], Volatility::Immutable),
Review Comment:
https://github.com/apache/datafusion/pull/12224/files#diff-71970189679c6dd5b3b677bb21603234b488e68d1601be9c4d400d40e430a909R70 pretty sure this line captures the LargeUtf8 usage at the moment.
--
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] Update the CONCAT scalar function to support Utf8View [datafusion]
Posted by "tshauck (via GitHub)" <gi...@apache.org>.
tshauck commented on code in PR #12224:
URL: https://github.com/apache/datafusion/pull/12224#discussion_r1737152586
##########
datafusion/functions/src/string/concat.rs:
##########
@@ -46,7 +45,7 @@ impl ConcatFunc {
pub fn new() -> Self {
use DataType::*;
Self {
- signature: Signature::variadic(vec![Utf8], Volatility::Immutable),
+ signature: Signature::variadic(vec![Utf8, Utf8View], Volatility::Immutable),
Review Comment:
Sorry, should've been clearer, to your point, I think it works but causes a cast to utf8.
```
> CREATE TABLE ttt AS SELECT arrow_cast('T', 'LargeUtf8') AS a;
0 row(s) fetched.
Elapsed 0.012 seconds.
> SELECt ttt.a FROM ttt;
+---+
| a |
+---+
| T |
+---+
1 row(s) fetched.
Elapsed 0.007 seconds.
> EXPLAIN SELECt ttt.a FROM ttt;
+---------------+-----------------------------------------------+
| plan_type | plan |
+---------------+-----------------------------------------------+
| logical_plan | TableScan: ttt projection=[a] |
| physical_plan | MemoryExec: partitions=1, partition_sizes=[1] |
| | |
+---------------+-----------------------------------------------+
2 row(s) fetched.
Elapsed 0.007 seconds.
> EXPLAIN SELECt concat(ttt.a, ttt.a) FROM ttt;
+---------------+--------------------------------------------------------------------------------------------+
| plan_type | plan |
+---------------+--------------------------------------------------------------------------------------------+
| logical_plan | Projection: concat(__common_expr_1 AS ttt.a, __common_expr_1 AS ttt.a) |
| | Projection: CAST(ttt.a AS Utf8) AS __common_expr_1 |
| | TableScan: ttt projection=[a] |
| physical_plan | ProjectionExec: expr=[concat(__common_expr_1@0, __common_expr_1@0) as concat(ttt.a,ttt.a)] |
| | ProjectionExec: expr=[CAST(a@0 AS Utf8) as __common_expr_1] |
| | MemoryExec: partitions=1, partition_sizes=[1] |
| | |
+---------------+--------------------------------------------------------------------------------------------+
```
I think if you were to concat two largeutf8s together, you should get that same type as the return?
--
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] Update the CONCAT scalar function to support Utf8View [datafusion]
Posted by "devanbenz (via GitHub)" <gi...@apache.org>.
devanbenz commented on code in PR #12224:
URL: https://github.com/apache/datafusion/pull/12224#discussion_r1738510028
##########
datafusion/functions/src/string/concat.rs:
##########
@@ -64,13 +66,19 @@ impl ScalarUDFImpl for ConcatFunc {
&self.signature
}
- fn return_type(&self, _arg_types: &[DataType]) -> Result<DataType> {
- Ok(Utf8)
+ fn return_type(&self, arg_types: &[DataType]) -> Result<DataType> {
+ use DataType::*;
+ Ok(match &arg_types[0] {
+ Utf8View => Utf8View,
+ LargeUtf8 => LargeUtf8,
+ _ => Utf8,
+ })
Review Comment:
Ah okay. So what you're saying is that here: https://github.com/apache/datafusion/pull/12224/files#diff-71970189679c6dd5b3b677bb21603234b488e68d1601be9c4d400d40e430a909R204 I'm building a Utf8 string anyways?
So I suspect I should change that bit of code to use a StringViewArrayBuilder?
--
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] Update the CONCAT scalar function to support Utf8View [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #12224:
URL: https://github.com/apache/datafusion/pull/12224
--
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] Update the CONCAT scalar function to support Utf8View [datafusion]
Posted by "devanbenz (via GitHub)" <gi...@apache.org>.
devanbenz commented on code in PR #12224:
URL: https://github.com/apache/datafusion/pull/12224#discussion_r1737586402
##########
datafusion/functions/src/string/concat.rs:
##########
@@ -64,13 +66,19 @@ impl ScalarUDFImpl for ConcatFunc {
&self.signature
}
- fn return_type(&self, _arg_types: &[DataType]) -> Result<DataType> {
- Ok(Utf8)
+ fn return_type(&self, arg_types: &[DataType]) -> Result<DataType> {
+ use DataType::*;
+ Ok(match &arg_types[0] {
+ Utf8View => Utf8View,
+ LargeUtf8 => LargeUtf8,
+ _ => Utf8,
+ })
Review Comment:
Ah yeah @findepi I think the logic is "Whatever the first argument type is the output should be of that type" so if the received values were: Utf8, Utf8View the output would be Utf8. I'm taking the logic from other UDFs and applying it here. It may not be the best way of doing this though.
--
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] Update the CONCAT scalar function to support Utf8View [datafusion]
Posted by "devanbenz (via GitHub)" <gi...@apache.org>.
devanbenz commented on code in PR #12224:
URL: https://github.com/apache/datafusion/pull/12224#discussion_r1738586565
##########
datafusion/functions/src/string/concat.rs:
##########
@@ -64,13 +66,19 @@ impl ScalarUDFImpl for ConcatFunc {
&self.signature
}
- fn return_type(&self, _arg_types: &[DataType]) -> Result<DataType> {
- Ok(Utf8)
+ fn return_type(&self, arg_types: &[DataType]) -> Result<DataType> {
+ use DataType::*;
+ Ok(match &arg_types[0] {
+ Utf8View => Utf8View,
+ LargeUtf8 => LargeUtf8,
+ _ => Utf8,
+ })
Review Comment:
@findepi what about for LargeUtf8? I suspect that if a LargeUtf8 is the input then the output should also be that since its an i64 datatype vs the i32 datatype for Utf8?
--
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] Update the CONCAT scalar function to support Utf8View [datafusion]
Posted by "devanbenz (via GitHub)" <gi...@apache.org>.
devanbenz commented on code in PR #12224:
URL: https://github.com/apache/datafusion/pull/12224#discussion_r1740164275
##########
datafusion/functions/src/string/concat.rs:
##########
@@ -64,13 +66,18 @@ impl ScalarUDFImpl for ConcatFunc {
&self.signature
}
- fn return_type(&self, _arg_types: &[DataType]) -> Result<DataType> {
- Ok(Utf8)
+ fn return_type(&self, arg_types: &[DataType]) -> Result<DataType> {
+ use DataType::*;
+ Ok(match &arg_types[0] {
+ LargeUtf8 => LargeUtf8,
+ _ => Utf8,
+ })
Review Comment:
I've added test cases for Utf8View and LargeUtf8 return types. Have gone ahead and adjusted this.
--
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] Update the CONCAT scalar function to support Utf8View [datafusion]
Posted by "findepi (via GitHub)" <gi...@apache.org>.
findepi commented on code in PR #12224:
URL: https://github.com/apache/datafusion/pull/12224#discussion_r1739649597
##########
datafusion/functions/src/string/concat.rs:
##########
@@ -64,13 +66,19 @@ impl ScalarUDFImpl for ConcatFunc {
&self.signature
}
- fn return_type(&self, _arg_types: &[DataType]) -> Result<DataType> {
- Ok(Utf8)
+ fn return_type(&self, arg_types: &[DataType]) -> Result<DataType> {
+ use DataType::*;
+ Ok(match &arg_types[0] {
+ Utf8View => Utf8View,
+ LargeUtf8 => LargeUtf8,
+ _ => Utf8,
+ })
Review Comment:
> when you say binary concat operator are you talking about `||` as an operator?
yes
> the logic seems to assume all arguments are of the same type?
>
> also, why not always return `Utf8`? the code performing actual concatenation seems to be always the same.
the question still holds (why exactly we bias towards the first param type), but i am no longer convinced about my suggestion to use Utf8 always.
i think we should "just" make sure `concat(a, b, c)` is type-equivalent to `a || b || c`.
the `||`'s logic apparently is
- if any of the operands are Utf8View, the result is Utf8View
- else, if any of the operands are LargeUtf8, the result is LargeUtf8
cc @alamb
--
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] Update the CONCAT scalar function to support Utf8View [datafusion]
Posted by "findepi (via GitHub)" <gi...@apache.org>.
findepi commented on code in PR #12224:
URL: https://github.com/apache/datafusion/pull/12224#discussion_r1739650055
##########
datafusion/functions/src/string/concat.rs:
##########
@@ -64,13 +66,18 @@ impl ScalarUDFImpl for ConcatFunc {
&self.signature
}
- fn return_type(&self, _arg_types: &[DataType]) -> Result<DataType> {
- Ok(Utf8)
+ fn return_type(&self, arg_types: &[DataType]) -> Result<DataType> {
+ use DataType::*;
+ Ok(match &arg_types[0] {
+ LargeUtf8 => LargeUtf8,
+ _ => Utf8,
+ })
Review Comment:
currently Utf8View isn't declared as an expected return type,
but the code does return Utf8View values.
should anything be failing when the runtime value type doesn't match the declared return type?
(if so, what would be a test case exercising such case?)
cc @comphead
--
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] Update the CONCAT scalar function to support Utf8View [datafusion]
Posted by "devanbenz (via GitHub)" <gi...@apache.org>.
devanbenz commented on code in PR #12224:
URL: https://github.com/apache/datafusion/pull/12224#discussion_r1737404565
##########
datafusion/functions/src/string/concat.rs:
##########
@@ -64,13 +66,19 @@ impl ScalarUDFImpl for ConcatFunc {
&self.signature
}
- fn return_type(&self, _arg_types: &[DataType]) -> Result<DataType> {
- Ok(Utf8)
+ fn return_type(&self, arg_types: &[DataType]) -> Result<DataType> {
+ use DataType::*;
+ Ok(match &arg_types[0] {
+ Utf8View => Utf8View,
+ LargeUtf8 => LargeUtf8,
+ _ => Utf8,
+ })
}
/// Concatenates the text representations of all the arguments. NULL arguments are ignored.
/// concat('abcde', 2, NULL, 22) = 'abcde222'
fn invoke(&self, args: &[ColumnarValue]) -> Result<ColumnarValue> {
+ let args_datatype = args[0].data_type();
Review Comment:
The concat function does not support zero arguments
For the second point yeah I can change 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] Update the CONCAT scalar function to support Utf8View [datafusion]
Posted by "devanbenz (via GitHub)" <gi...@apache.org>.
devanbenz commented on code in PR #12224:
URL: https://github.com/apache/datafusion/pull/12224#discussion_r1739707943
##########
datafusion/functions/src/string/concat.rs:
##########
@@ -64,13 +66,19 @@ impl ScalarUDFImpl for ConcatFunc {
&self.signature
}
- fn return_type(&self, _arg_types: &[DataType]) -> Result<DataType> {
- Ok(Utf8)
+ fn return_type(&self, arg_types: &[DataType]) -> Result<DataType> {
+ use DataType::*;
+ Ok(match &arg_types[0] {
+ Utf8View => Utf8View,
+ LargeUtf8 => LargeUtf8,
+ _ => Utf8,
+ })
Review Comment:
Sounds good @findepi i like that logic. I can adjust to make it so.
--
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] Update the CONCAT scalar function to support Utf8View [datafusion]
Posted by "devanbenz (via GitHub)" <gi...@apache.org>.
devanbenz commented on code in PR #12224:
URL: https://github.com/apache/datafusion/pull/12224#discussion_r1737157800
##########
datafusion/functions/src/string/concat.rs:
##########
@@ -46,7 +45,7 @@ impl ConcatFunc {
pub fn new() -> Self {
use DataType::*;
Self {
- signature: Signature::variadic(vec![Utf8], Volatility::Immutable),
+ signature: Signature::variadic(vec![Utf8, Utf8View], Volatility::Immutable),
Review Comment:
Correct - I will need to modify the input/return values for this. I'll add an additional test for LargeUtf8 and fix in this PR tonight. It shouldn't take long, I think I have a path forward to change that and it should be relatively painless.
--
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] Update the CONCAT scalar function to support Utf8View [datafusion]
Posted by "devanbenz (via GitHub)" <gi...@apache.org>.
devanbenz commented on code in PR #12224:
URL: https://github.com/apache/datafusion/pull/12224#discussion_r1737187052
##########
datafusion/functions/src/string/concat.rs:
##########
@@ -46,7 +45,7 @@ impl ConcatFunc {
pub fn new() -> Self {
use DataType::*;
Self {
- signature: Signature::variadic(vec![Utf8], Volatility::Immutable),
+ signature: Signature::variadic(vec![Utf8, Utf8View], Volatility::Immutable),
Review Comment:
Correct** I fixed it now it was really quick resolution lol
--
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] Update the CONCAT scalar function to support Utf8View [datafusion]
Posted by "devanbenz (via GitHub)" <gi...@apache.org>.
devanbenz commented on code in PR #12224:
URL: https://github.com/apache/datafusion/pull/12224#discussion_r1737123941
##########
datafusion/functions/src/string/concat.rs:
##########
@@ -46,7 +45,7 @@ impl ConcatFunc {
pub fn new() -> Self {
use DataType::*;
Self {
- signature: Signature::variadic(vec![Utf8], Volatility::Immutable),
+ signature: Signature::variadic(vec![Utf8, Utf8View], Volatility::Immutable),
Review Comment:
I think it's casting to Utf8 from LargeUtf8? Not sure. I can follow up and modify this code to include that though it should be relatively quick.
--
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] Update the CONCAT scalar function to support Utf8View [datafusion]
Posted by "devanbenz (via GitHub)" <gi...@apache.org>.
devanbenz commented on code in PR #12224:
URL: https://github.com/apache/datafusion/pull/12224#discussion_r1737592576
##########
datafusion/functions/src/string/common.rs:
##########
@@ -418,6 +451,88 @@ impl StringArrayBuilder {
}
}
+pub(crate) struct LargeStringArrayBuilder {
Review Comment:
I wanted to make the already existing `StringArrayBuilder` generic but was having issues 😢
--
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] Update the CONCAT scalar function to support Utf8View [datafusion]
Posted by "devanbenz (via GitHub)" <gi...@apache.org>.
devanbenz commented on PR #12224:
URL: https://github.com/apache/datafusion/pull/12224#issuecomment-2324820327
> Thank you @devanbenz @findepi and @tshauck
>
> I think this PR is good enough to merge, but I agree with @devanbenz 's observation [#12224 (files)](https://github.com/apache/datafusion/pull/12224/files#r1740156822) that StringViewArrayBuilder feels hacky.
>
> Specifically I think what is happening is that `StringArrayBuilder` is for a pretty specialized usecase, and thus so is `StringViewArrayBuilder`
>
> I filed [apache/arrow-rs#6347](https://github.com/apache/arrow-rs/issues/6347) upstream with a description of the use case to see if we can come up with something more general
Great sounds good @alamb 🙏
--
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