You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "Omega359 (via GitHub)" <gi...@apache.org> on 2024/11/14 01:14:54 UTC
[PR] Add support for Utf8View to string_to_array and array_to_string [datafusion]
Omega359 opened a new pull request, #13403:
URL: https://github.com/apache/datafusion/pull/13403
## Which issue does this PR close?
Closes #13383
## Rationale for this change
Completing support for Utf8View in all functions.
## What changes are included in this PR?
Code tests
## Are these changes tested?
Yes
## Are there any user-facing changes?
No
--
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] Add support for Utf8View to string_to_array and array_to_string [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #13403:
URL: https://github.com/apache/datafusion/pull/13403#discussion_r1851052692
##########
datafusion/functions-nested/src/string.rs:
##########
@@ -524,63 +687,90 @@ pub fn string_to_array_inner<T: OffsetSizeTrait>(args: &[ArrayRef]) -> Result<Ar
}
(Some(string), None) => {
string.chars().map(|c| c.to_string()).for_each(|c| {
- list_builder.values().append_value(c);
+ list_builder.values().append_value(c.as_str());
});
list_builder.append(true);
}
_ => list_builder.append(false), // null value
}
},
- );
+ )
}
-
- 3 => {
- let null_value_array = as_generic_string_array::<T>(&args[2])?;
- string_array
- .iter()
- .zip(delimiter_array.iter())
- .zip(null_value_array.iter())
- .for_each(|((string, delimiter), null_value)| {
- match (string, delimiter) {
- (Some(string), Some("")) => {
- if Some(string) == null_value {
+ Some(null_value_array) => string_array
+ .iter()
+ .zip(delimiter_array.iter())
+ .zip(null_value_array.iter())
+ .for_each(|((string, delimiter), null_value)| {
+ match (string, delimiter) {
+ (Some(string), Some("")) => {
+ if Some(string) == null_value {
+ list_builder.values().append_null();
+ } else {
+ list_builder.values().append_value(string);
+ }
+ list_builder.append(true);
+ }
+ (Some(string), Some(delimiter)) => {
+ string.split(delimiter).for_each(|s| {
+ if Some(s) == null_value {
list_builder.values().append_null();
} else {
- list_builder.values().append_value(string);
+ list_builder.values().append_value(s);
}
- list_builder.append(true);
- }
- (Some(string), Some(delimiter)) => {
- string.split(delimiter).for_each(|s| {
- if Some(s) == null_value {
- list_builder.values().append_null();
- } else {
- list_builder.values().append_value(s);
- }
- });
- list_builder.append(true);
- }
- (Some(string), None) => {
- string.chars().map(|c| c.to_string()).for_each(|c| {
- if Some(c.as_str()) == null_value {
- list_builder.values().append_null();
- } else {
- list_builder.values().append_value(c);
- }
- });
- list_builder.append(true);
- }
- _ => list_builder.append(false), // null value
+ });
+ list_builder.append(true);
}
- });
- }
- _ => {
- return exec_err!(
- "Expect string_to_array function to take two or three parameters"
- )
- }
- }
+ (Some(string), None) => {
+ string.chars().map(|c| c.to_string()).for_each(|c| {
+ if Some(c.as_str()) == null_value {
+ list_builder.values().append_null();
+ } else {
+ list_builder.values().append_value(c.as_str());
+ }
+ });
+ list_builder.append(true);
+ }
+ _ => list_builder.append(false), // null value
+ }
+ }),
+ };
let list_array = list_builder.finish();
Ok(Arc::new(list_array) as ArrayRef)
}
+
+trait StringArrayBuilderType: ArrayBuilder {
Review Comment:
this is an interesting idea -- maybe something worth looking into making more visible somehow / reusing to reduce duplication
##########
datafusion/functions-nested/src/string.rs:
##########
@@ -495,20 +505,173 @@ pub(super) fn array_to_string_inner(args: &[ArrayRef]) -> Result<ArrayRef> {
/// String_to_array SQL function
/// Splits string at occurrences of delimiter and returns an array of parts
/// string_to_array('abc~@~def~@~ghi', '~@~') = '["abc", "def", "ghi"]'
-pub fn string_to_array_inner<T: OffsetSizeTrait>(args: &[ArrayRef]) -> Result<ArrayRef> {
+fn string_to_array_inner<T: OffsetSizeTrait>(args: &[ArrayRef]) -> Result<ArrayRef> {
if args.len() < 2 || args.len() > 3 {
return exec_err!("string_to_array expects two or three arguments");
}
- let string_array = as_generic_string_array::<T>(&args[0])?;
- let delimiter_array = as_generic_string_array::<T>(&args[1])?;
- let mut list_builder = ListBuilder::new(StringBuilder::with_capacity(
- string_array.len(),
- string_array.get_buffer_memory_size(),
- ));
+ match args[0].data_type() {
+ Utf8 => {
+ let string_array = args[0].as_string::<T>();
+ let builder = StringBuilder::with_capacity(string_array.len(), string_array.get_buffer_memory_size());
+ string_to_array_inner_2::<&GenericStringArray<T>, StringBuilder>(args, string_array, builder)
+ }
+ Utf8View => {
+ let string_array = args[0].as_string_view();
+ let builder = StringViewBuilder::with_capacity(string_array.len());
+ string_to_array_inner_2::<&StringViewArray, StringViewBuilder>(args, string_array, builder)
+ }
+ LargeUtf8 => {
+ let string_array = args[0].as_string::<T>();
+ let builder = LargeStringBuilder::with_capacity(string_array.len(), string_array.get_buffer_memory_size());
+ string_to_array_inner_2::<&GenericStringArray<T>, LargeStringBuilder>(args, string_array, builder)
+ }
+ other => exec_err!("unsupported type for first argument to string_to_array function as {other:?}")
+ }
+}
+
+fn string_to_array_inner_2<'a, StringArrType, StringBuilderType>(
+ args: &'a [ArrayRef],
+ string_array: StringArrType,
+ string_builder: StringBuilderType,
+) -> Result<ArrayRef>
+where
+ StringArrType: StringArrayType<'a>,
+ StringBuilderType: StringArrayBuilderType,
+{
+ match args[1].data_type() {
+ Utf8 => {
+ let delimiter_array = args[1].as_string::<i32>();
+ if args.len() == 2 {
+ string_to_array_impl::<
+ StringArrType,
+ &GenericStringArray<i32>,
+ &StringViewArray,
+ StringBuilderType,
+ >(string_array, delimiter_array, None, string_builder)
+ } else {
+ string_to_array_inner_3::<StringArrType,
+ &GenericStringArray<i32>,
+ StringBuilderType>(args, string_array, delimiter_array, string_builder)
+ }
+ }
+ Utf8View => {
+ let delimiter_array = args[1].as_string_view();
+
+ if args.len() == 2 {
+ string_to_array_impl::<
+ StringArrType,
+ &StringViewArray,
+ &StringViewArray,
+ StringBuilderType,
+ >(string_array, delimiter_array, None, string_builder)
+ } else {
+ string_to_array_inner_3::<StringArrType,
+ &StringViewArray,
+ StringBuilderType>(args, string_array, delimiter_array, string_builder)
+ }
+ }
+ LargeUtf8 => {
+ let delimiter_array = args[1].as_string::<i64>();
+ if args.len() == 2 {
+ string_to_array_impl::<
+ StringArrType,
+ &GenericStringArray<i64>,
+ &StringViewArray,
+ StringBuilderType,
+ >(string_array, delimiter_array, None, string_builder)
+ } else {
+ string_to_array_inner_3::<StringArrType,
+ &GenericStringArray<i64>,
+ StringBuilderType>(args, string_array, delimiter_array, string_builder)
+ }
+ }
+ other => exec_err!("unsupported type for second argument to string_to_array function as {other:?}")
+ }
+}
- match args.len() {
- 2 => {
+fn string_to_array_inner_3<'a, StringArrType, DelimiterArrType, StringBuilderType>(
Review Comment:
If I am reading this correctly, this function effectively handles all possible combinations of `Utf8`, `Utf8View` and `LargeUtf8` by generating specialized implementations for each combination -- so I think that means 3 * 3 = 9 copies of the function
I wonder if it is common to mix all the types 🤔
##########
datafusion/sqllogictest/test_files/array.slt:
##########
@@ -6916,6 +6940,79 @@ select string_to_array(e, ',') from values;
[adipiscing]
NULL
+# karge string tests for string_to_array
Review Comment:
```suggestion
# large string tests for string_to_array
```
##########
datafusion/functions-nested/src/string.rs:
##########
@@ -495,20 +505,173 @@ pub(super) fn array_to_string_inner(args: &[ArrayRef]) -> Result<ArrayRef> {
/// String_to_array SQL function
/// Splits string at occurrences of delimiter and returns an array of parts
/// string_to_array('abc~@~def~@~ghi', '~@~') = '["abc", "def", "ghi"]'
-pub fn string_to_array_inner<T: OffsetSizeTrait>(args: &[ArrayRef]) -> Result<ArrayRef> {
+fn string_to_array_inner<T: OffsetSizeTrait>(args: &[ArrayRef]) -> Result<ArrayRef> {
if args.len() < 2 || args.len() > 3 {
return exec_err!("string_to_array expects two or three arguments");
}
- let string_array = as_generic_string_array::<T>(&args[0])?;
- let delimiter_array = as_generic_string_array::<T>(&args[1])?;
- let mut list_builder = ListBuilder::new(StringBuilder::with_capacity(
- string_array.len(),
- string_array.get_buffer_memory_size(),
- ));
+ match args[0].data_type() {
+ Utf8 => {
+ let string_array = args[0].as_string::<T>();
+ let builder = StringBuilder::with_capacity(string_array.len(), string_array.get_buffer_memory_size());
+ string_to_array_inner_2::<&GenericStringArray<T>, StringBuilder>(args, string_array, builder)
+ }
+ Utf8View => {
+ let string_array = args[0].as_string_view();
+ let builder = StringViewBuilder::with_capacity(string_array.len());
+ string_to_array_inner_2::<&StringViewArray, StringViewBuilder>(args, string_array, builder)
+ }
+ LargeUtf8 => {
+ let string_array = args[0].as_string::<T>();
+ let builder = LargeStringBuilder::with_capacity(string_array.len(), string_array.get_buffer_memory_size());
+ string_to_array_inner_2::<&GenericStringArray<T>, LargeStringBuilder>(args, string_array, builder)
+ }
+ other => exec_err!("unsupported type for first argument to string_to_array function as {other:?}")
+ }
+}
+
+fn string_to_array_inner_2<'a, StringArrType, StringBuilderType>(
Review Comment:
inner_2 😆
--
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] Add support for Utf8View to string_to_array and array_to_string [datafusion]
Posted by "Omega359 (via GitHub)" <gi...@apache.org>.
Omega359 commented on code in PR #13403:
URL: https://github.com/apache/datafusion/pull/13403#discussion_r1845164042
##########
datafusion/functions-nested/src/string.rs:
##########
@@ -88,6 +90,7 @@ macro_rules! call_array_function {
($DATATYPE:expr, $INCLUDE_LIST:expr) => {{
match $DATATYPE {
DataType::List(_) => array_function!(ListArray),
+ DataType::Utf8View => array_function!(StringViewArray),
Review Comment:
Curious why that would matter?
--
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] Add support for Utf8View to string_to_array and array_to_string [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #13403:
URL: https://github.com/apache/datafusion/pull/13403#issuecomment-2492398784
Let's go with this and iterate on improvements going forward
--
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] Add support for Utf8View to string_to_array and array_to_string [datafusion]
Posted by "findepi (via GitHub)" <gi...@apache.org>.
findepi commented on code in PR #13403:
URL: https://github.com/apache/datafusion/pull/13403#discussion_r1845182679
##########
datafusion/functions-nested/src/string.rs:
##########
@@ -251,20 +254,21 @@ impl ScalarUDFImpl for StringToArray {
Utf8 | LargeUtf8 => {
List(Arc::new(Field::new("item", arg_types[0].clone(), true)))
}
+ Utf8View => List(Arc::new(Field::new("item", Utf8, true))),
Review Comment:
yes, indeed. is contrary also possible?
also, isn't this the downstream function's problem, not here?
ideally in https://github.com/apache/datafusion/issues/12622, utf8view would be run-time variant of utf8, same logical type, so next function could choose on its own
--
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] Add support for Utf8View to string_to_array and array_to_string [datafusion]
Posted by "findepi (via GitHub)" <gi...@apache.org>.
findepi commented on code in PR #13403:
URL: https://github.com/apache/datafusion/pull/13403#discussion_r1845181616
##########
datafusion/functions-nested/src/string.rs:
##########
@@ -88,6 +90,7 @@ macro_rules! call_array_function {
($DATATYPE:expr, $INCLUDE_LIST:expr) => {{
match $DATATYPE {
DataType::List(_) => array_function!(ListArray),
+ DataType::Utf8View => array_function!(StringViewArray),
Review Comment:
if code is consistently ordered, it's easier to follow
it's obviously a nit, ok to ignore
--
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] Add support for Utf8View to string_to_array and array_to_string [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #13403:
URL: https://github.com/apache/datafusion/pull/13403#issuecomment-2491701015
> That is a very good point @alamb and one that I don't have an answer to. Is there any guidelines anywhere as to what should be covered and what shouldn't? From my reading of functions its all over the place with no consistency at all.
I agree -- there is no guideline that I know of. Any chance you would be willing to propose one?
--
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] Add support for Utf8View to string_to_array and array_to_string [datafusion]
Posted by "Omega359 (via GitHub)" <gi...@apache.org>.
Omega359 commented on code in PR #13403:
URL: https://github.com/apache/datafusion/pull/13403#discussion_r1842448505
##########
datafusion/functions-nested/src/string.rs:
##########
@@ -251,20 +254,21 @@ impl ScalarUDFImpl for StringToArray {
Utf8 | LargeUtf8 => {
List(Arc::new(Field::new("item", arg_types[0].clone(), true)))
}
+ Utf8View => List(Arc::new(Field::new("item", Utf8, true))),
Review Comment:
Possible though I am uncertain as to whether anything would benefit from it. Hardly any functions currently return Utf8View currently.
--
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] Add support for Utf8View to string_to_array and array_to_string [datafusion]
Posted by "Omega359 (via GitHub)" <gi...@apache.org>.
Omega359 commented on code in PR #13403:
URL: https://github.com/apache/datafusion/pull/13403#discussion_r1842448505
##########
datafusion/functions-nested/src/string.rs:
##########
@@ -251,20 +254,21 @@ impl ScalarUDFImpl for StringToArray {
Utf8 | LargeUtf8 => {
List(Arc::new(Field::new("item", arg_types[0].clone(), true)))
}
+ Utf8View => List(Arc::new(Field::new("item", Utf8, true))),
Review Comment:
Possible though I am uncertain as to whether anything would benefit from it. Hardly any functions currently return 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
Re: [PR] Add support for Utf8View to string_to_array and array_to_string [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #13403:
URL: https://github.com/apache/datafusion/pull/13403
--
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] Add support for Utf8View to string_to_array and array_to_string [datafusion]
Posted by "Omega359 (via GitHub)" <gi...@apache.org>.
Omega359 commented on PR #13403:
URL: https://github.com/apache/datafusion/pull/13403#issuecomment-2488651850
Ready for review again.
--
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] Add support for Utf8View to string_to_array and array_to_string [datafusion]
Posted by "Omega359 (via GitHub)" <gi...@apache.org>.
Omega359 commented on code in PR #13403:
URL: https://github.com/apache/datafusion/pull/13403#discussion_r1851135533
##########
datafusion/functions-nested/src/string.rs:
##########
@@ -524,63 +687,90 @@ pub fn string_to_array_inner<T: OffsetSizeTrait>(args: &[ArrayRef]) -> Result<Ar
}
(Some(string), None) => {
string.chars().map(|c| c.to_string()).for_each(|c| {
- list_builder.values().append_value(c);
+ list_builder.values().append_value(c.as_str());
});
list_builder.append(true);
}
_ => list_builder.append(false), // null value
}
},
- );
+ )
}
-
- 3 => {
- let null_value_array = as_generic_string_array::<T>(&args[2])?;
- string_array
- .iter()
- .zip(delimiter_array.iter())
- .zip(null_value_array.iter())
- .for_each(|((string, delimiter), null_value)| {
- match (string, delimiter) {
- (Some(string), Some("")) => {
- if Some(string) == null_value {
+ Some(null_value_array) => string_array
+ .iter()
+ .zip(delimiter_array.iter())
+ .zip(null_value_array.iter())
+ .for_each(|((string, delimiter), null_value)| {
+ match (string, delimiter) {
+ (Some(string), Some("")) => {
+ if Some(string) == null_value {
+ list_builder.values().append_null();
+ } else {
+ list_builder.values().append_value(string);
+ }
+ list_builder.append(true);
+ }
+ (Some(string), Some(delimiter)) => {
+ string.split(delimiter).for_each(|s| {
+ if Some(s) == null_value {
list_builder.values().append_null();
} else {
- list_builder.values().append_value(string);
+ list_builder.values().append_value(s);
}
- list_builder.append(true);
- }
- (Some(string), Some(delimiter)) => {
- string.split(delimiter).for_each(|s| {
- if Some(s) == null_value {
- list_builder.values().append_null();
- } else {
- list_builder.values().append_value(s);
- }
- });
- list_builder.append(true);
- }
- (Some(string), None) => {
- string.chars().map(|c| c.to_string()).for_each(|c| {
- if Some(c.as_str()) == null_value {
- list_builder.values().append_null();
- } else {
- list_builder.values().append_value(c);
- }
- });
- list_builder.append(true);
- }
- _ => list_builder.append(false), // null value
+ });
+ list_builder.append(true);
}
- });
- }
- _ => {
- return exec_err!(
- "Expect string_to_array function to take two or three parameters"
- )
- }
- }
+ (Some(string), None) => {
+ string.chars().map(|c| c.to_string()).for_each(|c| {
+ if Some(c.as_str()) == null_value {
+ list_builder.values().append_null();
+ } else {
+ list_builder.values().append_value(c.as_str());
+ }
+ });
+ list_builder.append(true);
+ }
+ _ => list_builder.append(false), // null value
+ }
+ }),
+ };
let list_array = list_builder.finish();
Ok(Arc::new(list_array) as ArrayRef)
}
+
+trait StringArrayBuilderType: ArrayBuilder {
Review Comment:
I really wish Rust had enum variants as types - with that I could really reduce a lot of this code. One of the reasons this took so long is that I was looking for a clean and fast way to handle the string types in a super nice way. I failed unfortunately - what I came come up with similar to https://github.com/Omega359/arrow-datafusion/blob/38d9bdde69381b4a7516346f862310700b6eb96e/datafusion/functions/src/utils.rs#L192 but that just has more overhead than is acceptable :(
--
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] Add support for Utf8View to string_to_array and array_to_string [datafusion]
Posted by "Omega359 (via GitHub)" <gi...@apache.org>.
Omega359 commented on code in PR #13403:
URL: https://github.com/apache/datafusion/pull/13403#discussion_r1845190135
##########
datafusion/functions-nested/src/string.rs:
##########
@@ -251,20 +254,21 @@ impl ScalarUDFImpl for StringToArray {
Utf8 | LargeUtf8 => {
List(Arc::new(Field::new("item", arg_types[0].clone(), true)))
}
+ Utf8View => List(Arc::new(Field::new("item", Utf8, true))),
Review Comment:
> yes, indeed. is contrary also possible? also, isn't this the downstream function's problem, not here?
True. However since few other functions return utf8vew (currently) it would generally mean that at some point any system that chains functions (fairly normal dataframe usage) the output is going to be utf8 anyways. I'm not against adding it here at all, just being realistic as to what conversions are going to happen in a lot of cases anyways.
We may want to create an epic to update all the functions such that the output matches the input type (if input and output are string types) similar to what we did for utf8view for input.
> ideally in #12622, utf8view would be run-time variant of utf8, same logical type, so next function could choose on its own
Yes, that would be the case for input.
--
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] Add support for Utf8View to string_to_array and array_to_string [datafusion]
Posted by "findepi (via GitHub)" <gi...@apache.org>.
findepi commented on code in PR #13403:
URL: https://github.com/apache/datafusion/pull/13403#discussion_r1845150639
##########
datafusion/functions-nested/src/string.rs:
##########
@@ -251,20 +254,21 @@ impl ScalarUDFImpl for StringToArray {
Utf8 | LargeUtf8 => {
List(Arc::new(Field::new("item", arg_types[0].clone(), true)))
}
+ Utf8View => List(Arc::new(Field::new("item", Utf8, true))),
Review Comment:
unclear to me -- if we're creating new string values, would there be benefit of using Utf8View?
##########
datafusion/functions-nested/src/string.rs:
##########
@@ -88,6 +90,7 @@ macro_rules! call_array_function {
($DATATYPE:expr, $INCLUDE_LIST:expr) => {{
match $DATATYPE {
DataType::List(_) => array_function!(ListArray),
+ DataType::Utf8View => array_function!(StringViewArray),
Review Comment:
nit: maybe put it _after_ DataType::Utf8 match branch
--
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] Add support for Utf8View to string_to_array and array_to_string [datafusion]
Posted by "Omega359 (via GitHub)" <gi...@apache.org>.
Omega359 commented on code in PR #13403:
URL: https://github.com/apache/datafusion/pull/13403#discussion_r1845170064
##########
datafusion/functions-nested/src/string.rs:
##########
@@ -251,20 +254,21 @@ impl ScalarUDFImpl for StringToArray {
Utf8 | LargeUtf8 => {
List(Arc::new(Field::new("item", arg_types[0].clone(), true)))
}
+ Utf8View => List(Arc::new(Field::new("item", Utf8, true))),
Review Comment:
If a function called after one of these functions were called that would actually materially benefit from utf8view vs utf8 would it really matter. The main difference in actual code would be requiring a different StringBuilder in the `string_to_array_impl` function.
--
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] Add support for Utf8View to string_to_array and array_to_string [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on code in PR #13403:
URL: https://github.com/apache/datafusion/pull/13403#discussion_r1841884150
##########
datafusion/functions-nested/src/string.rs:
##########
@@ -251,20 +254,21 @@ impl ScalarUDFImpl for StringToArray {
Utf8 | LargeUtf8 => {
List(Arc::new(Field::new("item", arg_types[0].clone(), true)))
}
+ Utf8View => List(Arc::new(Field::new("item", Utf8, true))),
Review Comment:
Should we use UTf8View for the element type?
--
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] Add support for Utf8View to string_to_array and array_to_string [datafusion]
Posted by "Omega359 (via GitHub)" <gi...@apache.org>.
Omega359 commented on PR #13403:
URL: https://github.com/apache/datafusion/pull/13403#issuecomment-2483441618
Do not merge - this PR does not properly handle largeutf8 and I want to try to refactor the code to reduce the # of lines of 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] Add support for Utf8View to string_to_array and array_to_string [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #13403:
URL: https://github.com/apache/datafusion/pull/13403#issuecomment-2484052694
Converting to draft while @Omega359 makes the changes described in https://github.com/apache/datafusion/pull/13403#issuecomment-2483441618
--
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] Add support for Utf8View to string_to_array and array_to_string [datafusion]
Posted by "Omega359 (via GitHub)" <gi...@apache.org>.
Omega359 commented on PR #13403:
URL: https://github.com/apache/datafusion/pull/13403#issuecomment-2494565464
> I agree -- there is no guideline that I know of. Any chance you would be willing to propose one?
I'll have to think about it tbh @alamb. Part of that is to know the cost to go from utf8 -> utf8View / utf8view -> utf8. I remember seeing a comment somewhere but my search-fu isn't advanced enough to find it again
--
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] Add support for Utf8View to string_to_array and array_to_string [datafusion]
Posted by "Omega359 (via GitHub)" <gi...@apache.org>.
Omega359 commented on PR #13403:
URL: https://github.com/apache/datafusion/pull/13403#issuecomment-2491314326
> I do wonder if we really need this level of specialization, but I also think since this is clearly better than what is on main we can/should merge it in
That is a very good point @alamb and one that I don't have an answer to. Is there any guidelines anywhere as to what should be covered and what shouldn't? From my reading of functions its all over the place with no consistency at all.
--
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