You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "Lordworms (via GitHub)" <gi...@apache.org> on 2024/08/12 04:23:03 UTC
[PR] Update RPAD scalar function to support Utf8View [datafusion]
Lordworms opened a new pull request, #11942:
URL: https://github.com/apache/datafusion/pull/11942
## 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 #11918
## 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 RPAD scalar function to support Utf8View [datafusion]
Posted by "Omega359 (via GitHub)" <gi...@apache.org>.
Omega359 commented on code in PR #11942:
URL: https://github.com/apache/datafusion/pull/11942#discussion_r1714181648
##########
datafusion/functions/src/unicode/rpad.rs:
##########
@@ -46,8 +48,11 @@ impl RPadFunc {
signature: Signature::one_of(
vec![
Exact(vec![Utf8, Int64]),
+ Exact(vec![Utf8View, Int64]),
Exact(vec![LargeUtf8, Int64]),
Exact(vec![Utf8, Int64, Utf8]),
+ Exact(vec![Utf8View, Int64, Utf8]),
+ Exact(vec![Utf8View, Int64, Utf8View]),
Review Comment:
Given the possible signatures I would think so. An example I can come up with is populating the different args from a table that have different column types, one being utf8 and another being utf8view. Not standard, no, but possible.
--
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 RPAD scalar function to support Utf8View [datafusion]
Posted by "Lordworms (via GitHub)" <gi...@apache.org>.
Lordworms commented on PR #11942:
URL: https://github.com/apache/datafusion/pull/11942#issuecomment-2284573250
> You may want to look at the tests I added in #11941 as an example to verify that all the signature variants are tested.
I may directly reference the test case there, is it ok?
--
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 RPAD scalar function to support Utf8View [datafusion]
Posted by "Lordworms (via GitHub)" <gi...@apache.org>.
Lordworms commented on code in PR #11942:
URL: https://github.com/apache/datafusion/pull/11942#discussion_r1714125884
##########
datafusion/functions/src/unicode/rpad.rs:
##########
@@ -77,96 +82,122 @@ impl ScalarUDFImpl for RPadFunc {
fn invoke(&self, args: &[ColumnarValue]) -> Result<ColumnarValue> {
match args[0].data_type() {
- DataType::Utf8 => make_scalar_function(rpad::<i32>, vec![])(args),
+ DataType::Utf8 | DataType::Utf8View => {
+ make_scalar_function(rpad::<i32>, vec![])(args)
+ }
DataType::LargeUtf8 => make_scalar_function(rpad::<i64>, vec![])(args),
other => exec_err!("Unsupported data type {other:?} for function rpad"),
}
}
}
-/// Extends the string to length 'length' by appending the characters fill (a space by default). If the string is already longer than length then it is truncated.
-/// rpad('hi', 5, 'xy') = 'hixyx'
-pub fn rpad<T: OffsetSizeTrait>(args: &[ArrayRef]) -> Result<ArrayRef> {
- match args.len() {
- 2 => {
- let string_array = as_generic_string_array::<T>(&args[0])?;
- let length_array = as_int64_array(&args[1])?;
-
- let result = string_array
- .iter()
- .zip(length_array.iter())
- .map(|(string, length)| match (string, length) {
- (Some(string), Some(length)) => {
- if length > i32::MAX as i64 {
- return exec_err!(
- "rpad requested length {length} too large"
- );
- }
+macro_rules! process_rpad {
+ // For the two-argument case
+ ($string_array:expr, $length_array:expr, $is_view:expr) => {{
+ $string_array
+ .iter()
+ .zip($length_array.iter())
+ .map(|(string, length)| match (string, length) {
+ (Some(string), Some(length)) => {
+ if length > i32::MAX as i64 {
+ return exec_err!("rpad requested length {} too large", length);
+ }
- let length = if length < 0 { 0 } else { length as usize };
- if length == 0 {
- Ok(Some("".to_string()))
+ let length = if length < 0 { 0 } else { length as usize };
+ if length == 0 {
+ Ok(Some("".to_string()))
+ } else {
+ let graphemes = string.graphemes(true).collect::<Vec<&str>>();
+ if length < graphemes.len() {
+ Ok(Some(graphemes[..length].concat()))
} else {
- let graphemes = string.graphemes(true).collect::<Vec<&str>>();
- if length < graphemes.len() {
- Ok(Some(graphemes[..length].concat()))
+ let mut s = string.to_string();
+ if !$is_view {
Review Comment:
I'll discard that, sorry for it
##########
datafusion/functions/src/unicode/rpad.rs:
##########
@@ -46,8 +48,11 @@ impl RPadFunc {
signature: Signature::one_of(
vec![
Exact(vec![Utf8, Int64]),
Review Comment:
Sure
--
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 RPAD scalar function to support Utf8View [datafusion]
Posted by "Lordworms (via GitHub)" <gi...@apache.org>.
Lordworms commented on code in PR #11942:
URL: https://github.com/apache/datafusion/pull/11942#discussion_r1714275162
##########
datafusion/functions/src/unicode/rpad.rs:
##########
@@ -46,8 +48,11 @@ impl RPadFunc {
signature: Signature::one_of(
vec![
Exact(vec![Utf8, Int64]),
+ Exact(vec![Utf8View, Int64]),
Exact(vec![LargeUtf8, Int64]),
Exact(vec![Utf8, Int64, Utf8]),
+ Exact(vec![Utf8View, Int64, Utf8]),
+ Exact(vec![Utf8View, Int64, Utf8View]),
Review Comment:
I have added the logics
--
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 RPAD scalar function to support Utf8View [datafusion]
Posted by "Omega359 (via GitHub)" <gi...@apache.org>.
Omega359 commented on code in PR #11942:
URL: https://github.com/apache/datafusion/pull/11942#discussion_r1715698875
##########
datafusion/functions/src/unicode/rpad.rs:
##########
@@ -76,97 +84,168 @@ impl ScalarUDFImpl for RPadFunc {
}
fn invoke(&self, args: &[ColumnarValue]) -> Result<ColumnarValue> {
- match args[0].data_type() {
- DataType::Utf8 => make_scalar_function(rpad::<i32>, vec![])(args),
- DataType::LargeUtf8 => make_scalar_function(rpad::<i64>, vec![])(args),
- other => exec_err!("Unsupported data type {other:?} for function rpad"),
+ match args.len() {
+ 2 => match args[0].data_type() {
+ DataType::Utf8 | DataType::Utf8View => {
+ make_scalar_function(rpad::<i32, i32>, vec![])(args)
+ }
+ DataType::LargeUtf8 => {
+ make_scalar_function(rpad::<i64, i64>, vec![])(args)
+ }
+ other => exec_err!("Unsupported data type {other:?} for function rpad"),
+ },
+ 3 => match (args[0].data_type(), args[2].data_type()) {
+ (
+ DataType::Utf8 | DataType::Utf8View,
+ DataType::Utf8 | DataType::Utf8View,
Review Comment:
This line looks to be a duplicate.
--
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 RPAD scalar function to support Utf8View [datafusion]
Posted by "Lordworms (via GitHub)" <gi...@apache.org>.
Lordworms commented on code in PR #11942:
URL: https://github.com/apache/datafusion/pull/11942#discussion_r1715830931
##########
datafusion/functions/src/unicode/rpad.rs:
##########
@@ -76,97 +84,168 @@ impl ScalarUDFImpl for RPadFunc {
}
fn invoke(&self, args: &[ColumnarValue]) -> Result<ColumnarValue> {
- match args[0].data_type() {
- DataType::Utf8 => make_scalar_function(rpad::<i32>, vec![])(args),
- DataType::LargeUtf8 => make_scalar_function(rpad::<i64>, vec![])(args),
- other => exec_err!("Unsupported data type {other:?} for function rpad"),
+ match args.len() {
+ 2 => match args[0].data_type() {
+ DataType::Utf8 | DataType::Utf8View => {
+ make_scalar_function(rpad::<i32, i32>, vec![])(args)
+ }
+ DataType::LargeUtf8 => {
+ make_scalar_function(rpad::<i64, i64>, vec![])(args)
+ }
+ other => exec_err!("Unsupported data type {other:?} for function rpad"),
+ },
+ 3 => match (args[0].data_type(), args[2].data_type()) {
+ (
+ DataType::Utf8 | DataType::Utf8View,
+ DataType::Utf8 | DataType::Utf8View,
+ ) => make_scalar_function(rpad::<i32, i32>, vec![])(args),
+ (DataType::LargeUtf8, DataType::LargeUtf8) => {
+ make_scalar_function(rpad::<i64, i64>, vec![])(args)
+ }
+ (DataType::LargeUtf8, DataType::Utf8View | DataType::Utf8) => {
+ make_scalar_function(rpad::<i64, i32>, vec![])(args)
+ }
+ (DataType::Utf8View | DataType::Utf8, DataType::LargeUtf8) => {
+ make_scalar_function(rpad::<i32, i64>, vec![])(args)
+ }
+ (first_type, last_type) => {
+ exec_err!("unsupported arguments type for rpad, first argument type is {}, last argument type is {}", first_type, last_type)
+ }
+ },
+ number => {
+ exec_err!("unsupported arguments number {} for rpad", number)
+ }
}
}
}
-/// Extends the string to length 'length' by appending the characters fill (a space by default). If the string is already longer than length then it is truncated.
-/// rpad('hi', 5, 'xy') = 'hixyx'
-pub fn rpad<T: OffsetSizeTrait>(args: &[ArrayRef]) -> Result<ArrayRef> {
- match args.len() {
- 2 => {
- let string_array = as_generic_string_array::<T>(&args[0])?;
- let length_array = as_int64_array(&args[1])?;
-
- let result = string_array
- .iter()
- .zip(length_array.iter())
- .map(|(string, length)| match (string, length) {
- (Some(string), Some(length)) => {
- if length > i32::MAX as i64 {
- return exec_err!(
- "rpad requested length {length} too large"
- );
- }
-
- let length = if length < 0 { 0 } else { length as usize };
- if length == 0 {
- Ok(Some("".to_string()))
- } else {
- let graphemes = string.graphemes(true).collect::<Vec<&str>>();
- if length < graphemes.len() {
- Ok(Some(graphemes[..length].concat()))
- } else {
- let mut s = string.to_string();
- s.push_str(" ".repeat(length - graphemes.len()).as_str());
- Ok(Some(s))
- }
- }
+macro_rules! process_rpad {
+ // For the two-argument case
+ ($string_array:expr, $length_array:expr, $is_view:expr) => {{
Review Comment:
Sure, I'll remove it soon
--
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 RPAD scalar function to support Utf8View [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #11942:
URL: https://github.com/apache/datafusion/pull/11942#issuecomment-2286989094
Thank you @Lordworms and @Omega359
--
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 RPAD scalar function to support Utf8View [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #11942:
URL: https://github.com/apache/datafusion/pull/11942#issuecomment-2289911392
> Looks good to me @Lordworms and @Omega359
>
> An epic journey of macros 🚀
>
> I can't help but think this function could be made much faster by using a StringBuilder rather than creating a bunch of `String`s which are then copied.
>
> However, that could be some future pR
@Omega359's PR here is a pretty good example of how to do this https://github.com/apache/datafusion/pull/11987
--
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 RPAD scalar function to support Utf8View [datafusion]
Posted by "Omega359 (via GitHub)" <gi...@apache.org>.
Omega359 commented on PR #11942:
URL: https://github.com/apache/datafusion/pull/11942#issuecomment-2284082623
You may want to look at the tests I added in https://github.com/apache/datafusion/pull/11941 as an example to verify that all the signature variants are tested.
--
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 RPAD scalar function to support Utf8View [datafusion]
Posted by "Lordworms (via GitHub)" <gi...@apache.org>.
Lordworms commented on code in PR #11942:
URL: https://github.com/apache/datafusion/pull/11942#discussion_r1714125884
##########
datafusion/functions/src/unicode/rpad.rs:
##########
@@ -77,96 +82,122 @@ impl ScalarUDFImpl for RPadFunc {
fn invoke(&self, args: &[ColumnarValue]) -> Result<ColumnarValue> {
match args[0].data_type() {
- DataType::Utf8 => make_scalar_function(rpad::<i32>, vec![])(args),
+ DataType::Utf8 | DataType::Utf8View => {
+ make_scalar_function(rpad::<i32>, vec![])(args)
+ }
DataType::LargeUtf8 => make_scalar_function(rpad::<i64>, vec![])(args),
other => exec_err!("Unsupported data type {other:?} for function rpad"),
}
}
}
-/// Extends the string to length 'length' by appending the characters fill (a space by default). If the string is already longer than length then it is truncated.
-/// rpad('hi', 5, 'xy') = 'hixyx'
-pub fn rpad<T: OffsetSizeTrait>(args: &[ArrayRef]) -> Result<ArrayRef> {
- match args.len() {
- 2 => {
- let string_array = as_generic_string_array::<T>(&args[0])?;
- let length_array = as_int64_array(&args[1])?;
-
- let result = string_array
- .iter()
- .zip(length_array.iter())
- .map(|(string, length)| match (string, length) {
- (Some(string), Some(length)) => {
- if length > i32::MAX as i64 {
- return exec_err!(
- "rpad requested length {length} too large"
- );
- }
+macro_rules! process_rpad {
+ // For the two-argument case
+ ($string_array:expr, $length_array:expr, $is_view:expr) => {{
+ $string_array
+ .iter()
+ .zip($length_array.iter())
+ .map(|(string, length)| match (string, length) {
+ (Some(string), Some(length)) => {
+ if length > i32::MAX as i64 {
+ return exec_err!("rpad requested length {} too large", length);
+ }
- let length = if length < 0 { 0 } else { length as usize };
- if length == 0 {
- Ok(Some("".to_string()))
+ let length = if length < 0 { 0 } else { length as usize };
+ if length == 0 {
+ Ok(Some("".to_string()))
+ } else {
+ let graphemes = string.graphemes(true).collect::<Vec<&str>>();
+ if length < graphemes.len() {
+ Ok(Some(graphemes[..length].concat()))
} else {
- let graphemes = string.graphemes(true).collect::<Vec<&str>>();
- if length < graphemes.len() {
- Ok(Some(graphemes[..length].concat()))
+ let mut s = string.to_string();
+ if !$is_view {
Review Comment:
I'll discard that, sorry for it, it is the old code implementation and I copied this to make it a macro
--
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 RPAD scalar function to support Utf8View [datafusion]
Posted by "Omega359 (via GitHub)" <gi...@apache.org>.
Omega359 commented on PR #11942:
URL: https://github.com/apache/datafusion/pull/11942#issuecomment-2284640583
> > You may want to look at the tests I added in #11941 as an example to verify that all the signature variants are tested.
>
> I may directly reference the test case there, is it ok?
It currently hardcodes the LPadFunc in it so it'll require a refactor but given how close these two functions are I can't see an issue off the top of my head.
--
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 RPAD scalar function to support Utf8View [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #11942:
URL: https://github.com/apache/datafusion/pull/11942#issuecomment-2289928000
Filed
> > > Looks good to me @Lordworms and @Omega359
> > > An epic journey of macros 🚀
> > > I can't help but think this function could be made much faster by using a StringBuilder rather than creating a bunch of `String`s which are then copied.
> > > However, that could be some future pR
> >
> >
> > @Omega359's PR here is a pretty good example of how to do this #11987
>
> Sure, I'll add a PR for it tomorrow.
Filed https://github.com/apache/datafusion/issues/11997 to track
--
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 RPAD scalar function to support Utf8View [datafusion]
Posted by "Omega359 (via GitHub)" <gi...@apache.org>.
Omega359 commented on code in PR #11942:
URL: https://github.com/apache/datafusion/pull/11942#discussion_r1713779456
##########
datafusion/functions/src/unicode/rpad.rs:
##########
@@ -77,96 +82,122 @@ impl ScalarUDFImpl for RPadFunc {
fn invoke(&self, args: &[ColumnarValue]) -> Result<ColumnarValue> {
match args[0].data_type() {
- DataType::Utf8 => make_scalar_function(rpad::<i32>, vec![])(args),
+ DataType::Utf8 | DataType::Utf8View => {
+ make_scalar_function(rpad::<i32>, vec![])(args)
+ }
DataType::LargeUtf8 => make_scalar_function(rpad::<i64>, vec![])(args),
other => exec_err!("Unsupported data type {other:?} for function rpad"),
}
}
}
-/// Extends the string to length 'length' by appending the characters fill (a space by default). If the string is already longer than length then it is truncated.
-/// rpad('hi', 5, 'xy') = 'hixyx'
-pub fn rpad<T: OffsetSizeTrait>(args: &[ArrayRef]) -> Result<ArrayRef> {
- match args.len() {
- 2 => {
- let string_array = as_generic_string_array::<T>(&args[0])?;
- let length_array = as_int64_array(&args[1])?;
-
- let result = string_array
- .iter()
- .zip(length_array.iter())
- .map(|(string, length)| match (string, length) {
- (Some(string), Some(length)) => {
- if length > i32::MAX as i64 {
- return exec_err!(
- "rpad requested length {length} too large"
- );
- }
+macro_rules! process_rpad {
+ // For the two-argument case
+ ($string_array:expr, $length_array:expr, $is_view:expr) => {{
+ $string_array
+ .iter()
+ .zip($length_array.iter())
+ .map(|(string, length)| match (string, length) {
+ (Some(string), Some(length)) => {
+ if length > i32::MAX as i64 {
+ return exec_err!("rpad requested length {} too large", length);
+ }
- let length = if length < 0 { 0 } else { length as usize };
- if length == 0 {
- Ok(Some("".to_string()))
+ let length = if length < 0 { 0 } else { length as usize };
+ if length == 0 {
+ Ok(Some("".to_string()))
+ } else {
+ let graphemes = string.graphemes(true).collect::<Vec<&str>>();
+ if length < graphemes.len() {
+ Ok(Some(graphemes[..length].concat()))
} else {
- let graphemes = string.graphemes(true).collect::<Vec<&str>>();
- if length < graphemes.len() {
- Ok(Some(graphemes[..length].concat()))
+ let mut s = string.to_string();
+ if !$is_view {
+ s.push_str(&" ".repeat(length - graphemes.len()));
} else {
- let mut s = string.to_string();
s.push_str(" ".repeat(length - graphemes.len()).as_str());
- Ok(Some(s))
}
+ Ok(Some(s))
}
}
- _ => Ok(None),
- })
- .collect::<Result<GenericStringArray<T>>>()?;
+ }
+ _ => Ok(None),
+ })
+ .collect::<Result<GenericStringArray<T>>>()
+ }};
+
+ // For the three-argument case
+ ($string_array:expr, $length_array:expr, $fill_array:expr, $is_view:expr) => {{
+ $string_array
+ .iter()
+ .zip($length_array.iter())
+ .zip($fill_array.iter())
+ .map(|((string, length), fill)| match (string, length, fill) {
+ (Some(string), Some(length), Some(fill)) => {
+ if length > i32::MAX as i64 {
+ return exec_err!("rpad requested length {} too large", length);
+ }
+
+ let length = if length < 0 { 0 } else { length as usize };
+ let graphemes = string.graphemes(true).collect::<Vec<&str>>();
+ let fill_chars = fill.chars().collect::<Vec<char>>();
+
+ if length < graphemes.len() {
+ Ok(Some(graphemes[..length].concat()))
+ } else if fill_chars.is_empty() {
+ Ok(Some(string.to_string()))
+ } else {
+ let mut s = string.to_string();
+ let char_vector: Vec<char> = (0..length - graphemes.len())
+ .map(|l| fill_chars[l % fill_chars.len()])
+ .collect();
+ s.push_str(&char_vector.iter().collect::<String>());
+ Ok(Some(s))
+ }
+ }
+ _ => Ok(None),
+ })
+ .collect::<Result<GenericStringArray<T>>>()
Review Comment:
lpad was the same here iirc
--
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 RPAD scalar function to support Utf8View [datafusion]
Posted by "Omega359 (via GitHub)" <gi...@apache.org>.
Omega359 commented on code in PR #11942:
URL: https://github.com/apache/datafusion/pull/11942#discussion_r1713832682
##########
datafusion/functions/src/unicode/rpad.rs:
##########
@@ -77,96 +82,122 @@ impl ScalarUDFImpl for RPadFunc {
fn invoke(&self, args: &[ColumnarValue]) -> Result<ColumnarValue> {
match args[0].data_type() {
- DataType::Utf8 => make_scalar_function(rpad::<i32>, vec![])(args),
+ DataType::Utf8 | DataType::Utf8View => {
+ make_scalar_function(rpad::<i32>, vec![])(args)
+ }
DataType::LargeUtf8 => make_scalar_function(rpad::<i64>, vec![])(args),
other => exec_err!("Unsupported data type {other:?} for function rpad"),
}
}
}
-/// Extends the string to length 'length' by appending the characters fill (a space by default). If the string is already longer than length then it is truncated.
-/// rpad('hi', 5, 'xy') = 'hixyx'
-pub fn rpad<T: OffsetSizeTrait>(args: &[ArrayRef]) -> Result<ArrayRef> {
- match args.len() {
- 2 => {
- let string_array = as_generic_string_array::<T>(&args[0])?;
- let length_array = as_int64_array(&args[1])?;
-
- let result = string_array
- .iter()
- .zip(length_array.iter())
- .map(|(string, length)| match (string, length) {
- (Some(string), Some(length)) => {
- if length > i32::MAX as i64 {
- return exec_err!(
- "rpad requested length {length} too large"
- );
- }
+macro_rules! process_rpad {
+ // For the two-argument case
+ ($string_array:expr, $length_array:expr, $is_view:expr) => {{
+ $string_array
+ .iter()
+ .zip($length_array.iter())
+ .map(|(string, length)| match (string, length) {
+ (Some(string), Some(length)) => {
+ if length > i32::MAX as i64 {
+ return exec_err!("rpad requested length {} too large", length);
+ }
- let length = if length < 0 { 0 } else { length as usize };
- if length == 0 {
- Ok(Some("".to_string()))
+ let length = if length < 0 { 0 } else { length as usize };
+ if length == 0 {
+ Ok(Some("".to_string()))
+ } else {
+ let graphemes = string.graphemes(true).collect::<Vec<&str>>();
+ if length < graphemes.len() {
+ Ok(Some(graphemes[..length].concat()))
} else {
- let graphemes = string.graphemes(true).collect::<Vec<&str>>();
- if length < graphemes.len() {
- Ok(Some(graphemes[..length].concat()))
+ let mut s = string.to_string();
+ if !$is_view {
+ s.push_str(&" ".repeat(length - graphemes.len()));
} else {
- let mut s = string.to_string();
s.push_str(" ".repeat(length - graphemes.len()).as_str());
- Ok(Some(s))
}
+ Ok(Some(s))
}
}
- _ => Ok(None),
- })
- .collect::<Result<GenericStringArray<T>>>()?;
+ }
+ _ => Ok(None),
+ })
+ .collect::<Result<GenericStringArray<T>>>()
+ }};
+
+ // For the three-argument case
+ ($string_array:expr, $length_array:expr, $fill_array:expr, $is_view:expr) => {{
+ $string_array
+ .iter()
+ .zip($length_array.iter())
+ .zip($fill_array.iter())
+ .map(|((string, length), fill)| match (string, length, fill) {
+ (Some(string), Some(length), Some(fill)) => {
+ if length > i32::MAX as i64 {
+ return exec_err!("rpad requested length {} too large", length);
+ }
+
+ let length = if length < 0 { 0 } else { length as usize };
+ let graphemes = string.graphemes(true).collect::<Vec<&str>>();
+ let fill_chars = fill.chars().collect::<Vec<char>>();
+
+ if length < graphemes.len() {
+ Ok(Some(graphemes[..length].concat()))
+ } else if fill_chars.is_empty() {
+ Ok(Some(string.to_string()))
+ } else {
+ let mut s = string.to_string();
+ let char_vector: Vec<char> = (0..length - graphemes.len())
+ .map(|l| fill_chars[l % fill_chars.len()])
+ .collect();
+ s.push_str(&char_vector.iter().collect::<String>());
+ Ok(Some(s))
+ }
+ }
+ _ => Ok(None),
+ })
+ .collect::<Result<GenericStringArray<T>>>()
+ }};
+}
+
+/// Extends the string to length 'length' by appending the characters fill (a space by default). If the string is already longer than length then it is truncated.
+/// rpad('hi', 5, 'xy') = 'hixyx'
+pub fn rpad<T: OffsetSizeTrait>(args: &[ArrayRef]) -> Result<ArrayRef> {
+ match (args.len(), args[0].data_type()) {
+ (2, DataType::Utf8View) => {
+ let string_array = as_string_view_array(&args[0])?;
+ let length_array = as_int64_array(&args[1])?;
+
+ let result = process_rpad!(string_array, length_array, true)?;
Ok(Arc::new(result) as ArrayRef)
}
- 3 => {
+ (2, _) => {
let string_array = as_generic_string_array::<T>(&args[0])?;
let length_array = as_int64_array(&args[1])?;
- let fill_array = as_generic_string_array::<T>(&args[2])?;
- let result = string_array
- .iter()
- .zip(length_array.iter())
- .zip(fill_array.iter())
- .map(|((string, length), fill)| match (string, length, fill) {
- (Some(string), Some(length), Some(fill)) => {
- if length > i32::MAX as i64 {
- return exec_err!(
- "rpad requested length {length} too large"
- );
- }
-
- let length = if length < 0 { 0 } else { length as usize };
- let graphemes = string.graphemes(true).collect::<Vec<&str>>();
- let fill_chars = fill.chars().collect::<Vec<char>>();
+ let result = process_rpad!(string_array, length_array, true)?;
+ Ok(Arc::new(result) as ArrayRef)
+ }
+ (3, DataType::Utf8View) => {
+ let string_array = as_string_view_array(&args[0])?;
+ let length_array = as_int64_array(&args[1])?;
+ let fill_array = as_string_view_array(&args[2])?;
Review Comment:
This code makes the assumption that the type of arg[0] and arg[2] are the same. I am unsure as to whether that is guaranteed or not. Thoughts?
##########
datafusion/functions/src/unicode/rpad.rs:
##########
@@ -46,8 +48,11 @@ impl RPadFunc {
signature: Signature::one_of(
vec![
Exact(vec![Utf8, Int64]),
+ Exact(vec![Utf8View, Int64]),
Exact(vec![LargeUtf8, Int64]),
Exact(vec![Utf8, Int64, Utf8]),
+ Exact(vec![Utf8View, Int64, Utf8]),
+ Exact(vec![Utf8View, Int64, Utf8View]),
Review Comment:
Should we not also have
`Exact(vec![Utf8, Int64, Utf8View])`,
`Exact(vec![LargeUtf8, Int64, Utf8View])`
etc?
--
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 RPAD scalar function to support Utf8View [datafusion]
Posted by "Omega359 (via GitHub)" <gi...@apache.org>.
Omega359 commented on code in PR #11942:
URL: https://github.com/apache/datafusion/pull/11942#discussion_r1713789099
##########
datafusion/functions/src/unicode/rpad.rs:
##########
@@ -46,8 +48,11 @@ impl RPadFunc {
signature: Signature::one_of(
vec![
Exact(vec![Utf8, Int64]),
Review Comment:
As noted in https://github.com/apache/datafusion/pull/11924/files::
// Planner attempts coercion to the target type starting with the most preferred candidate.
// For example, given input `(Utf8View, Utf8)`, it first tries coercing to `(Utf8View, Utf8View)`.
// If that fails, it proceeds to `(Utf8, Utf8)`.
Thus, the ordering of the signature should likely be changed 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 RPAD scalar function to support Utf8View [datafusion]
Posted by "Lordworms (via GitHub)" <gi...@apache.org>.
Lordworms commented on code in PR #11942:
URL: https://github.com/apache/datafusion/pull/11942#discussion_r1714127307
##########
datafusion/functions/src/unicode/rpad.rs:
##########
@@ -77,96 +82,122 @@ impl ScalarUDFImpl for RPadFunc {
fn invoke(&self, args: &[ColumnarValue]) -> Result<ColumnarValue> {
match args[0].data_type() {
- DataType::Utf8 => make_scalar_function(rpad::<i32>, vec![])(args),
+ DataType::Utf8 | DataType::Utf8View => {
+ make_scalar_function(rpad::<i32>, vec![])(args)
+ }
DataType::LargeUtf8 => make_scalar_function(rpad::<i64>, vec![])(args),
other => exec_err!("Unsupported data type {other:?} for function rpad"),
}
}
}
-/// Extends the string to length 'length' by appending the characters fill (a space by default). If the string is already longer than length then it is truncated.
-/// rpad('hi', 5, 'xy') = 'hixyx'
-pub fn rpad<T: OffsetSizeTrait>(args: &[ArrayRef]) -> Result<ArrayRef> {
- match args.len() {
- 2 => {
- let string_array = as_generic_string_array::<T>(&args[0])?;
- let length_array = as_int64_array(&args[1])?;
-
- let result = string_array
- .iter()
- .zip(length_array.iter())
- .map(|(string, length)| match (string, length) {
- (Some(string), Some(length)) => {
- if length > i32::MAX as i64 {
- return exec_err!(
- "rpad requested length {length} too large"
- );
- }
+macro_rules! process_rpad {
+ // For the two-argument case
+ ($string_array:expr, $length_array:expr, $is_view:expr) => {{
+ $string_array
+ .iter()
+ .zip($length_array.iter())
+ .map(|(string, length)| match (string, length) {
+ (Some(string), Some(length)) => {
+ if length > i32::MAX as i64 {
+ return exec_err!("rpad requested length {} too large", length);
+ }
- let length = if length < 0 { 0 } else { length as usize };
- if length == 0 {
- Ok(Some("".to_string()))
+ let length = if length < 0 { 0 } else { length as usize };
+ if length == 0 {
+ Ok(Some("".to_string()))
+ } else {
+ let graphemes = string.graphemes(true).collect::<Vec<&str>>();
+ if length < graphemes.len() {
+ Ok(Some(graphemes[..length].concat()))
} else {
- let graphemes = string.graphemes(true).collect::<Vec<&str>>();
- if length < graphemes.len() {
- Ok(Some(graphemes[..length].concat()))
+ let mut s = string.to_string();
+ if !$is_view {
+ s.push_str(&" ".repeat(length - graphemes.len()));
} else {
- let mut s = string.to_string();
s.push_str(" ".repeat(length - graphemes.len()).as_str());
- Ok(Some(s))
}
+ Ok(Some(s))
}
}
- _ => Ok(None),
- })
- .collect::<Result<GenericStringArray<T>>>()?;
+ }
+ _ => Ok(None),
+ })
+ .collect::<Result<GenericStringArray<T>>>()
+ }};
+
+ // For the three-argument case
+ ($string_array:expr, $length_array:expr, $fill_array:expr, $is_view:expr) => {{
+ $string_array
+ .iter()
+ .zip($length_array.iter())
+ .zip($fill_array.iter())
+ .map(|((string, length), fill)| match (string, length, fill) {
+ (Some(string), Some(length), Some(fill)) => {
+ if length > i32::MAX as i64 {
+ return exec_err!("rpad requested length {} too large", length);
+ }
+
+ let length = if length < 0 { 0 } else { length as usize };
+ let graphemes = string.graphemes(true).collect::<Vec<&str>>();
+ let fill_chars = fill.chars().collect::<Vec<char>>();
+
+ if length < graphemes.len() {
+ Ok(Some(graphemes[..length].concat()))
+ } else if fill_chars.is_empty() {
+ Ok(Some(string.to_string()))
+ } else {
+ let mut s = string.to_string();
+ let char_vector: Vec<char> = (0..length - graphemes.len())
+ .map(|l| fill_chars[l % fill_chars.len()])
+ .collect();
+ s.push_str(&char_vector.iter().collect::<String>());
+ Ok(Some(s))
+ }
+ }
+ _ => Ok(None),
+ })
+ .collect::<Result<GenericStringArray<T>>>()
+ }};
+}
+
+/// Extends the string to length 'length' by appending the characters fill (a space by default). If the string is already longer than length then it is truncated.
+/// rpad('hi', 5, 'xy') = 'hixyx'
+pub fn rpad<T: OffsetSizeTrait>(args: &[ArrayRef]) -> Result<ArrayRef> {
+ match (args.len(), args[0].data_type()) {
+ (2, DataType::Utf8View) => {
+ let string_array = as_string_view_array(&args[0])?;
+ let length_array = as_int64_array(&args[1])?;
+
+ let result = process_rpad!(string_array, length_array, true)?;
Ok(Arc::new(result) as ArrayRef)
}
- 3 => {
+ (2, _) => {
let string_array = as_generic_string_array::<T>(&args[0])?;
let length_array = as_int64_array(&args[1])?;
- let fill_array = as_generic_string_array::<T>(&args[2])?;
- let result = string_array
- .iter()
- .zip(length_array.iter())
- .zip(fill_array.iter())
- .map(|((string, length), fill)| match (string, length, fill) {
- (Some(string), Some(length), Some(fill)) => {
- if length > i32::MAX as i64 {
- return exec_err!(
- "rpad requested length {length} too large"
- );
- }
-
- let length = if length < 0 { 0 } else { length as usize };
- let graphemes = string.graphemes(true).collect::<Vec<&str>>();
- let fill_chars = fill.chars().collect::<Vec<char>>();
+ let result = process_rpad!(string_array, length_array, true)?;
+ Ok(Arc::new(result) as ArrayRef)
+ }
+ (3, DataType::Utf8View) => {
+ let string_array = as_string_view_array(&args[0])?;
+ let length_array = as_int64_array(&args[1])?;
+ let fill_array = as_string_view_array(&args[2])?;
Review Comment:
I'll fix 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 RPAD scalar function to support Utf8View [datafusion]
Posted by "Omega359 (via GitHub)" <gi...@apache.org>.
Omega359 commented on code in PR #11942:
URL: https://github.com/apache/datafusion/pull/11942#discussion_r1715698875
##########
datafusion/functions/src/unicode/rpad.rs:
##########
@@ -76,97 +84,168 @@ impl ScalarUDFImpl for RPadFunc {
}
fn invoke(&self, args: &[ColumnarValue]) -> Result<ColumnarValue> {
- match args[0].data_type() {
- DataType::Utf8 => make_scalar_function(rpad::<i32>, vec![])(args),
- DataType::LargeUtf8 => make_scalar_function(rpad::<i64>, vec![])(args),
- other => exec_err!("Unsupported data type {other:?} for function rpad"),
+ match args.len() {
+ 2 => match args[0].data_type() {
+ DataType::Utf8 | DataType::Utf8View => {
+ make_scalar_function(rpad::<i32, i32>, vec![])(args)
+ }
+ DataType::LargeUtf8 => {
+ make_scalar_function(rpad::<i64, i64>, vec![])(args)
+ }
+ other => exec_err!("Unsupported data type {other:?} for function rpad"),
+ },
+ 3 => match (args[0].data_type(), args[2].data_type()) {
+ (
+ DataType::Utf8 | DataType::Utf8View,
+ DataType::Utf8 | DataType::Utf8View,
Review Comment:
This line looks to be a duplicate. Correction, my mistake, ignore.
##########
datafusion/functions/src/unicode/rpad.rs:
##########
@@ -76,97 +84,168 @@ impl ScalarUDFImpl for RPadFunc {
}
fn invoke(&self, args: &[ColumnarValue]) -> Result<ColumnarValue> {
- match args[0].data_type() {
- DataType::Utf8 => make_scalar_function(rpad::<i32>, vec![])(args),
- DataType::LargeUtf8 => make_scalar_function(rpad::<i64>, vec![])(args),
- other => exec_err!("Unsupported data type {other:?} for function rpad"),
+ match args.len() {
+ 2 => match args[0].data_type() {
+ DataType::Utf8 | DataType::Utf8View => {
+ make_scalar_function(rpad::<i32, i32>, vec![])(args)
+ }
+ DataType::LargeUtf8 => {
+ make_scalar_function(rpad::<i64, i64>, vec![])(args)
+ }
+ other => exec_err!("Unsupported data type {other:?} for function rpad"),
+ },
+ 3 => match (args[0].data_type(), args[2].data_type()) {
+ (
+ DataType::Utf8 | DataType::Utf8View,
+ DataType::Utf8 | DataType::Utf8View,
Review Comment:
This line looks to be a duplicate. Correction, my mistake, 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] Update RPAD scalar function to support Utf8View [datafusion]
Posted by "Lordworms (via GitHub)" <gi...@apache.org>.
Lordworms commented on PR #11942:
URL: https://github.com/apache/datafusion/pull/11942#issuecomment-2284526203
> You may want to look at the tests I added in #11941 as an example to verify that all the signature variants are tested.
I'll add that, thanks for reminding
--
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 RPAD scalar function to support Utf8View [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #11942:
URL: https://github.com/apache/datafusion/pull/11942#discussion_r1713532418
##########
datafusion/functions/src/unicode/rpad.rs:
##########
@@ -77,96 +82,122 @@ impl ScalarUDFImpl for RPadFunc {
fn invoke(&self, args: &[ColumnarValue]) -> Result<ColumnarValue> {
match args[0].data_type() {
- DataType::Utf8 => make_scalar_function(rpad::<i32>, vec![])(args),
+ DataType::Utf8 | DataType::Utf8View => {
+ make_scalar_function(rpad::<i32>, vec![])(args)
+ }
DataType::LargeUtf8 => make_scalar_function(rpad::<i64>, vec![])(args),
other => exec_err!("Unsupported data type {other:?} for function rpad"),
}
}
}
-/// Extends the string to length 'length' by appending the characters fill (a space by default). If the string is already longer than length then it is truncated.
-/// rpad('hi', 5, 'xy') = 'hixyx'
-pub fn rpad<T: OffsetSizeTrait>(args: &[ArrayRef]) -> Result<ArrayRef> {
- match args.len() {
- 2 => {
- let string_array = as_generic_string_array::<T>(&args[0])?;
- let length_array = as_int64_array(&args[1])?;
-
- let result = string_array
- .iter()
- .zip(length_array.iter())
- .map(|(string, length)| match (string, length) {
- (Some(string), Some(length)) => {
- if length > i32::MAX as i64 {
- return exec_err!(
- "rpad requested length {length} too large"
- );
- }
+macro_rules! process_rpad {
+ // For the two-argument case
+ ($string_array:expr, $length_array:expr, $is_view:expr) => {{
+ $string_array
+ .iter()
+ .zip($length_array.iter())
+ .map(|(string, length)| match (string, length) {
+ (Some(string), Some(length)) => {
+ if length > i32::MAX as i64 {
+ return exec_err!("rpad requested length {} too large", length);
+ }
- let length = if length < 0 { 0 } else { length as usize };
- if length == 0 {
- Ok(Some("".to_string()))
+ let length = if length < 0 { 0 } else { length as usize };
+ if length == 0 {
+ Ok(Some("".to_string()))
+ } else {
+ let graphemes = string.graphemes(true).collect::<Vec<&str>>();
+ if length < graphemes.len() {
+ Ok(Some(graphemes[..length].concat()))
} else {
- let graphemes = string.graphemes(true).collect::<Vec<&str>>();
- if length < graphemes.len() {
- Ok(Some(graphemes[..length].concat()))
+ let mut s = string.to_string();
+ if !$is_view {
+ s.push_str(&" ".repeat(length - graphemes.len()));
} else {
- let mut s = string.to_string();
s.push_str(" ".repeat(length - graphemes.len()).as_str());
- Ok(Some(s))
}
+ Ok(Some(s))
}
}
- _ => Ok(None),
- })
- .collect::<Result<GenericStringArray<T>>>()?;
+ }
+ _ => Ok(None),
+ })
+ .collect::<Result<GenericStringArray<T>>>()
+ }};
+
+ // For the three-argument case
+ ($string_array:expr, $length_array:expr, $fill_array:expr, $is_view:expr) => {{
+ $string_array
+ .iter()
+ .zip($length_array.iter())
+ .zip($fill_array.iter())
+ .map(|((string, length), fill)| match (string, length, fill) {
+ (Some(string), Some(length), Some(fill)) => {
+ if length > i32::MAX as i64 {
+ return exec_err!("rpad requested length {} too large", length);
+ }
+
+ let length = if length < 0 { 0 } else { length as usize };
+ let graphemes = string.graphemes(true).collect::<Vec<&str>>();
+ let fill_chars = fill.chars().collect::<Vec<char>>();
+
+ if length < graphemes.len() {
+ Ok(Some(graphemes[..length].concat()))
+ } else if fill_chars.is_empty() {
+ Ok(Some(string.to_string()))
+ } else {
+ let mut s = string.to_string();
+ let char_vector: Vec<char> = (0..length - graphemes.len())
+ .map(|l| fill_chars[l % fill_chars.len()])
+ .collect();
+ s.push_str(&char_vector.iter().collect::<String>());
+ Ok(Some(s))
+ }
+ }
+ _ => Ok(None),
+ })
+ .collect::<Result<GenericStringArray<T>>>()
Review Comment:
This means the result of `rpad` is always `GenericStringArray` (`Utf8Array`) right?
I wonder if it would make sense to produce the same output type as the first argument 🤔 For the string view case eventually I could imagine reusing the same underlying buffers if the string had nothing to pad.
##########
datafusion/functions/src/unicode/rpad.rs:
##########
@@ -77,96 +82,122 @@ impl ScalarUDFImpl for RPadFunc {
fn invoke(&self, args: &[ColumnarValue]) -> Result<ColumnarValue> {
match args[0].data_type() {
- DataType::Utf8 => make_scalar_function(rpad::<i32>, vec![])(args),
+ DataType::Utf8 | DataType::Utf8View => {
+ make_scalar_function(rpad::<i32>, vec![])(args)
+ }
DataType::LargeUtf8 => make_scalar_function(rpad::<i64>, vec![])(args),
other => exec_err!("Unsupported data type {other:?} for function rpad"),
}
}
}
-/// Extends the string to length 'length' by appending the characters fill (a space by default). If the string is already longer than length then it is truncated.
-/// rpad('hi', 5, 'xy') = 'hixyx'
-pub fn rpad<T: OffsetSizeTrait>(args: &[ArrayRef]) -> Result<ArrayRef> {
- match args.len() {
- 2 => {
- let string_array = as_generic_string_array::<T>(&args[0])?;
- let length_array = as_int64_array(&args[1])?;
-
- let result = string_array
- .iter()
- .zip(length_array.iter())
- .map(|(string, length)| match (string, length) {
- (Some(string), Some(length)) => {
- if length > i32::MAX as i64 {
- return exec_err!(
- "rpad requested length {length} too large"
- );
- }
+macro_rules! process_rpad {
+ // For the two-argument case
+ ($string_array:expr, $length_array:expr, $is_view:expr) => {{
+ $string_array
+ .iter()
+ .zip($length_array.iter())
+ .map(|(string, length)| match (string, length) {
+ (Some(string), Some(length)) => {
+ if length > i32::MAX as i64 {
+ return exec_err!("rpad requested length {} too large", length);
+ }
- let length = if length < 0 { 0 } else { length as usize };
- if length == 0 {
- Ok(Some("".to_string()))
+ let length = if length < 0 { 0 } else { length as usize };
+ if length == 0 {
+ Ok(Some("".to_string()))
+ } else {
+ let graphemes = string.graphemes(true).collect::<Vec<&str>>();
+ if length < graphemes.len() {
+ Ok(Some(graphemes[..length].concat()))
} else {
- let graphemes = string.graphemes(true).collect::<Vec<&str>>();
- if length < graphemes.len() {
- Ok(Some(graphemes[..length].concat()))
+ let mut s = string.to_string();
+ if !$is_view {
Review Comment:
I don't understand the need to check $is_view here. I tried removing this clause and everything seemed to be good
--
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 RPAD scalar function to support Utf8View [datafusion]
Posted by "Lordworms (via GitHub)" <gi...@apache.org>.
Lordworms commented on PR #11942:
URL: https://github.com/apache/datafusion/pull/11942#issuecomment-2289924718
> > Looks good to me @Lordworms and @Omega359
> > An epic journey of macros 🚀
> > I can't help but think this function could be made much faster by using a StringBuilder rather than creating a bunch of `String`s which are then copied.
> > However, that could be some future pR
>
> @Omega359's PR here is a pretty good example of how to do this #11987
Sure, I'll add a PR for it tomorrow.
--
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 RPAD scalar function to support Utf8View [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #11942:
URL: https://github.com/apache/datafusion/pull/11942#discussion_r1715826438
##########
datafusion/functions/src/unicode/rpad.rs:
##########
@@ -76,97 +84,168 @@ impl ScalarUDFImpl for RPadFunc {
}
fn invoke(&self, args: &[ColumnarValue]) -> Result<ColumnarValue> {
- match args[0].data_type() {
- DataType::Utf8 => make_scalar_function(rpad::<i32>, vec![])(args),
- DataType::LargeUtf8 => make_scalar_function(rpad::<i64>, vec![])(args),
- other => exec_err!("Unsupported data type {other:?} for function rpad"),
+ match args.len() {
+ 2 => match args[0].data_type() {
+ DataType::Utf8 | DataType::Utf8View => {
+ make_scalar_function(rpad::<i32, i32>, vec![])(args)
+ }
+ DataType::LargeUtf8 => {
+ make_scalar_function(rpad::<i64, i64>, vec![])(args)
+ }
+ other => exec_err!("Unsupported data type {other:?} for function rpad"),
+ },
+ 3 => match (args[0].data_type(), args[2].data_type()) {
+ (
+ DataType::Utf8 | DataType::Utf8View,
+ DataType::Utf8 | DataType::Utf8View,
+ ) => make_scalar_function(rpad::<i32, i32>, vec![])(args),
+ (DataType::LargeUtf8, DataType::LargeUtf8) => {
+ make_scalar_function(rpad::<i64, i64>, vec![])(args)
+ }
+ (DataType::LargeUtf8, DataType::Utf8View | DataType::Utf8) => {
+ make_scalar_function(rpad::<i64, i32>, vec![])(args)
+ }
+ (DataType::Utf8View | DataType::Utf8, DataType::LargeUtf8) => {
+ make_scalar_function(rpad::<i32, i64>, vec![])(args)
+ }
+ (first_type, last_type) => {
+ exec_err!("unsupported arguments type for rpad, first argument type is {}, last argument type is {}", first_type, last_type)
+ }
+ },
+ number => {
+ exec_err!("unsupported arguments number {} for rpad", number)
+ }
}
}
}
-/// Extends the string to length 'length' by appending the characters fill (a space by default). If the string is already longer than length then it is truncated.
-/// rpad('hi', 5, 'xy') = 'hixyx'
-pub fn rpad<T: OffsetSizeTrait>(args: &[ArrayRef]) -> Result<ArrayRef> {
- match args.len() {
- 2 => {
- let string_array = as_generic_string_array::<T>(&args[0])?;
- let length_array = as_int64_array(&args[1])?;
-
- let result = string_array
- .iter()
- .zip(length_array.iter())
- .map(|(string, length)| match (string, length) {
- (Some(string), Some(length)) => {
- if length > i32::MAX as i64 {
- return exec_err!(
- "rpad requested length {length} too large"
- );
- }
-
- let length = if length < 0 { 0 } else { length as usize };
- if length == 0 {
- Ok(Some("".to_string()))
- } else {
- let graphemes = string.graphemes(true).collect::<Vec<&str>>();
- if length < graphemes.len() {
- Ok(Some(graphemes[..length].concat()))
- } else {
- let mut s = string.to_string();
- s.push_str(" ".repeat(length - graphemes.len()).as_str());
- Ok(Some(s))
- }
- }
+macro_rules! process_rpad {
+ // For the two-argument case
+ ($string_array:expr, $length_array:expr, $is_view:expr) => {{
Review Comment:
I agree -- removing it would simplify this PR
--
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 RPAD scalar function to support Utf8View [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #11942:
URL: https://github.com/apache/datafusion/pull/11942
--
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 RPAD scalar function to support Utf8View [datafusion]
Posted by "Lordworms (via GitHub)" <gi...@apache.org>.
Lordworms commented on code in PR #11942:
URL: https://github.com/apache/datafusion/pull/11942#discussion_r1714167156
##########
datafusion/functions/src/unicode/rpad.rs:
##########
@@ -46,8 +48,11 @@ impl RPadFunc {
signature: Signature::one_of(
vec![
Exact(vec![Utf8, Int64]),
+ Exact(vec![Utf8View, Int64]),
Exact(vec![LargeUtf8, Int64]),
Exact(vec![Utf8, Int64, Utf8]),
+ Exact(vec![Utf8View, Int64, Utf8]),
+ Exact(vec![Utf8View, Int64, Utf8View]),
Review Comment:
I have no idea here, I mean we could make the string array and fill array different type, but don't know if it is a proper behaviour
--
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 RPAD scalar function to support Utf8View [datafusion]
Posted by "Omega359 (via GitHub)" <gi...@apache.org>.
Omega359 commented on code in PR #11942:
URL: https://github.com/apache/datafusion/pull/11942#discussion_r1715564886
##########
datafusion/functions/src/unicode/rpad.rs:
##########
@@ -76,97 +84,168 @@ impl ScalarUDFImpl for RPadFunc {
}
fn invoke(&self, args: &[ColumnarValue]) -> Result<ColumnarValue> {
- match args[0].data_type() {
- DataType::Utf8 => make_scalar_function(rpad::<i32>, vec![])(args),
- DataType::LargeUtf8 => make_scalar_function(rpad::<i64>, vec![])(args),
- other => exec_err!("Unsupported data type {other:?} for function rpad"),
+ match args.len() {
+ 2 => match args[0].data_type() {
+ DataType::Utf8 | DataType::Utf8View => {
+ make_scalar_function(rpad::<i32, i32>, vec![])(args)
+ }
+ DataType::LargeUtf8 => {
+ make_scalar_function(rpad::<i64, i64>, vec![])(args)
+ }
+ other => exec_err!("Unsupported data type {other:?} for function rpad"),
+ },
+ 3 => match (args[0].data_type(), args[2].data_type()) {
+ (
+ DataType::Utf8 | DataType::Utf8View,
+ DataType::Utf8 | DataType::Utf8View,
+ ) => make_scalar_function(rpad::<i32, i32>, vec![])(args),
+ (DataType::LargeUtf8, DataType::LargeUtf8) => {
+ make_scalar_function(rpad::<i64, i64>, vec![])(args)
+ }
+ (DataType::LargeUtf8, DataType::Utf8View | DataType::Utf8) => {
+ make_scalar_function(rpad::<i64, i32>, vec![])(args)
+ }
+ (DataType::Utf8View | DataType::Utf8, DataType::LargeUtf8) => {
+ make_scalar_function(rpad::<i32, i64>, vec![])(args)
+ }
+ (first_type, last_type) => {
+ exec_err!("unsupported arguments type for rpad, first argument type is {}, last argument type is {}", first_type, last_type)
+ }
+ },
+ number => {
+ exec_err!("unsupported arguments number {} for rpad", number)
+ }
}
}
}
-/// Extends the string to length 'length' by appending the characters fill (a space by default). If the string is already longer than length then it is truncated.
-/// rpad('hi', 5, 'xy') = 'hixyx'
-pub fn rpad<T: OffsetSizeTrait>(args: &[ArrayRef]) -> Result<ArrayRef> {
- match args.len() {
- 2 => {
- let string_array = as_generic_string_array::<T>(&args[0])?;
- let length_array = as_int64_array(&args[1])?;
-
- let result = string_array
- .iter()
- .zip(length_array.iter())
- .map(|(string, length)| match (string, length) {
- (Some(string), Some(length)) => {
- if length > i32::MAX as i64 {
- return exec_err!(
- "rpad requested length {length} too large"
- );
- }
-
- let length = if length < 0 { 0 } else { length as usize };
- if length == 0 {
- Ok(Some("".to_string()))
- } else {
- let graphemes = string.graphemes(true).collect::<Vec<&str>>();
- if length < graphemes.len() {
- Ok(Some(graphemes[..length].concat()))
- } else {
- let mut s = string.to_string();
- s.push_str(" ".repeat(length - graphemes.len()).as_str());
- Ok(Some(s))
- }
- }
+macro_rules! process_rpad {
+ // For the two-argument case
+ ($string_array:expr, $length_array:expr, $is_view:expr) => {{
+ $string_array
+ .iter()
+ .zip($length_array.iter())
+ .map(|(string, length)| match (string, length) {
+ (Some(string), Some(length)) => {
+ if length > i32::MAX as i64 {
+ return exec_err!("rpad requested length {} too large", length);
}
- _ => Ok(None),
- })
- .collect::<Result<GenericStringArray<T>>>()?;
- Ok(Arc::new(result) as ArrayRef)
- }
- 3 => {
- let string_array = as_generic_string_array::<T>(&args[0])?;
- let length_array = as_int64_array(&args[1])?;
- let fill_array = as_generic_string_array::<T>(&args[2])?;
-
- let result = string_array
- .iter()
- .zip(length_array.iter())
- .zip(fill_array.iter())
- .map(|((string, length), fill)| match (string, length, fill) {
- (Some(string), Some(length), Some(fill)) => {
- if length > i32::MAX as i64 {
- return exec_err!(
- "rpad requested length {length} too large"
- );
- }
- let length = if length < 0 { 0 } else { length as usize };
+ let length = if length < 0 { 0 } else { length as usize };
+ if length == 0 {
+ Ok(Some("".to_string()))
+ } else {
let graphemes = string.graphemes(true).collect::<Vec<&str>>();
- let fill_chars = fill.chars().collect::<Vec<char>>();
-
if length < graphemes.len() {
Ok(Some(graphemes[..length].concat()))
- } else if fill_chars.is_empty() {
- Ok(Some(string.to_string()))
} else {
let mut s = string.to_string();
- let mut char_vector =
- Vec::<char>::with_capacity(length - graphemes.len());
- for l in 0..length - graphemes.len() {
- char_vector
- .push(*fill_chars.get(l % fill_chars.len()).unwrap());
- }
- s.push_str(char_vector.iter().collect::<String>().as_str());
+ s.push_str(" ".repeat(length - graphemes.len()).as_str());
Ok(Some(s))
}
}
- _ => Ok(None),
- })
- .collect::<Result<GenericStringArray<T>>>()?;
+ }
+ _ => Ok(None),
+ })
+ .collect::<Result<GenericStringArray<StringArrayLen>>>()
+ }};
+ // For the three-argument case
+ ($string_array:expr, $length_array:expr, $fill_array:expr, $is_view:expr) => {{
Review Comment:
is_view looks to be unused and it always passed in true. Should be removed unless I missed something.
##########
datafusion/functions/src/unicode/rpad.rs:
##########
@@ -76,97 +84,168 @@ impl ScalarUDFImpl for RPadFunc {
}
fn invoke(&self, args: &[ColumnarValue]) -> Result<ColumnarValue> {
- match args[0].data_type() {
- DataType::Utf8 => make_scalar_function(rpad::<i32>, vec![])(args),
- DataType::LargeUtf8 => make_scalar_function(rpad::<i64>, vec![])(args),
- other => exec_err!("Unsupported data type {other:?} for function rpad"),
+ match args.len() {
+ 2 => match args[0].data_type() {
+ DataType::Utf8 | DataType::Utf8View => {
+ make_scalar_function(rpad::<i32, i32>, vec![])(args)
+ }
+ DataType::LargeUtf8 => {
+ make_scalar_function(rpad::<i64, i64>, vec![])(args)
+ }
+ other => exec_err!("Unsupported data type {other:?} for function rpad"),
+ },
+ 3 => match (args[0].data_type(), args[2].data_type()) {
+ (
+ DataType::Utf8 | DataType::Utf8View,
+ DataType::Utf8 | DataType::Utf8View,
+ ) => make_scalar_function(rpad::<i32, i32>, vec![])(args),
+ (DataType::LargeUtf8, DataType::LargeUtf8) => {
+ make_scalar_function(rpad::<i64, i64>, vec![])(args)
+ }
+ (DataType::LargeUtf8, DataType::Utf8View | DataType::Utf8) => {
+ make_scalar_function(rpad::<i64, i32>, vec![])(args)
+ }
+ (DataType::Utf8View | DataType::Utf8, DataType::LargeUtf8) => {
+ make_scalar_function(rpad::<i32, i64>, vec![])(args)
+ }
+ (first_type, last_type) => {
+ exec_err!("unsupported arguments type for rpad, first argument type is {}, last argument type is {}", first_type, last_type)
+ }
+ },
+ number => {
+ exec_err!("unsupported arguments number {} for rpad", number)
+ }
}
}
}
-/// Extends the string to length 'length' by appending the characters fill (a space by default). If the string is already longer than length then it is truncated.
-/// rpad('hi', 5, 'xy') = 'hixyx'
-pub fn rpad<T: OffsetSizeTrait>(args: &[ArrayRef]) -> Result<ArrayRef> {
- match args.len() {
- 2 => {
- let string_array = as_generic_string_array::<T>(&args[0])?;
- let length_array = as_int64_array(&args[1])?;
-
- let result = string_array
- .iter()
- .zip(length_array.iter())
- .map(|(string, length)| match (string, length) {
- (Some(string), Some(length)) => {
- if length > i32::MAX as i64 {
- return exec_err!(
- "rpad requested length {length} too large"
- );
- }
-
- let length = if length < 0 { 0 } else { length as usize };
- if length == 0 {
- Ok(Some("".to_string()))
- } else {
- let graphemes = string.graphemes(true).collect::<Vec<&str>>();
- if length < graphemes.len() {
- Ok(Some(graphemes[..length].concat()))
- } else {
- let mut s = string.to_string();
- s.push_str(" ".repeat(length - graphemes.len()).as_str());
- Ok(Some(s))
- }
- }
+macro_rules! process_rpad {
+ // For the two-argument case
+ ($string_array:expr, $length_array:expr, $is_view:expr) => {{
Review Comment:
is_view looks to be unused and it always passed in true. Should be removed unless I missed something.
--
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 RPAD scalar function to support Utf8View [datafusion]
Posted by "Lordworms (via GitHub)" <gi...@apache.org>.
Lordworms commented on code in PR #11942:
URL: https://github.com/apache/datafusion/pull/11942#discussion_r1714125884
##########
datafusion/functions/src/unicode/rpad.rs:
##########
@@ -77,96 +82,122 @@ impl ScalarUDFImpl for RPadFunc {
fn invoke(&self, args: &[ColumnarValue]) -> Result<ColumnarValue> {
match args[0].data_type() {
- DataType::Utf8 => make_scalar_function(rpad::<i32>, vec![])(args),
+ DataType::Utf8 | DataType::Utf8View => {
+ make_scalar_function(rpad::<i32>, vec![])(args)
+ }
DataType::LargeUtf8 => make_scalar_function(rpad::<i64>, vec![])(args),
other => exec_err!("Unsupported data type {other:?} for function rpad"),
}
}
}
-/// Extends the string to length 'length' by appending the characters fill (a space by default). If the string is already longer than length then it is truncated.
-/// rpad('hi', 5, 'xy') = 'hixyx'
-pub fn rpad<T: OffsetSizeTrait>(args: &[ArrayRef]) -> Result<ArrayRef> {
- match args.len() {
- 2 => {
- let string_array = as_generic_string_array::<T>(&args[0])?;
- let length_array = as_int64_array(&args[1])?;
-
- let result = string_array
- .iter()
- .zip(length_array.iter())
- .map(|(string, length)| match (string, length) {
- (Some(string), Some(length)) => {
- if length > i32::MAX as i64 {
- return exec_err!(
- "rpad requested length {length} too large"
- );
- }
+macro_rules! process_rpad {
+ // For the two-argument case
+ ($string_array:expr, $length_array:expr, $is_view:expr) => {{
+ $string_array
+ .iter()
+ .zip($length_array.iter())
+ .map(|(string, length)| match (string, length) {
+ (Some(string), Some(length)) => {
+ if length > i32::MAX as i64 {
+ return exec_err!("rpad requested length {} too large", length);
+ }
- let length = if length < 0 { 0 } else { length as usize };
- if length == 0 {
- Ok(Some("".to_string()))
+ let length = if length < 0 { 0 } else { length as usize };
+ if length == 0 {
+ Ok(Some("".to_string()))
+ } else {
+ let graphemes = string.graphemes(true).collect::<Vec<&str>>();
+ if length < graphemes.len() {
+ Ok(Some(graphemes[..length].concat()))
} else {
- let graphemes = string.graphemes(true).collect::<Vec<&str>>();
- if length < graphemes.len() {
- Ok(Some(graphemes[..length].concat()))
+ let mut s = string.to_string();
+ if !$is_view {
Review Comment:
I'll discard that, sorry for it, it is the old code implementation
--
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