Skip to content

Make lower and upper emit Utf8View for Utf8View input #20616

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
alamb merged 6 commits into from
Mar 9, 2026
Merged

Make lower and upper emit Utf8View for Utf8View input #20616

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
837f896
Make lower emit Utf8View for Utf8View input
kumarUjjawal Feb 28, 2026
d92a711
Make lower and upper emit Utf8View for Utf8View input
kumarUjjawal Mar 2, 2026
d553ac1
Handle dictionary Utf8View return type in case conversion
kumarUjjawal Mar 2, 2026
825b423
Refine lower/upper Utf8View tests and return typing
kumarUjjawal Mar 3, 2026
5adb958
add dictionary slt tests
kumarUjjawal Mar 3, 2026
4ddfc61
remove case_conversion block
kumarUjjawal Mar 6, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 21 additions & 6 deletions datafusion/functions/src/string/common.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,10 @@
use std::sync::Arc;

use crate::strings::make_and_append_view;
use crate::utils::utf8_to_str_type;
use arrow::array::{
Array, ArrayRef, GenericStringArray, GenericStringBuilder, NullBufferBuilder,
OffsetSizeTrait, StringBuilder, StringViewArray, new_null_array,
OffsetSizeTrait, StringViewArray, StringViewBuilder, new_null_array,
};
use arrow::buffer::{Buffer, ScalarBuffer};
use arrow::datatypes::DataType;
Expand Down Expand Up @@ -331,6 +332,22 @@ fn string_trim<T: OffsetSizeTrait, Tr: Trimmer>(args: &[ArrayRef]) -> Result<Arr
}
}

pub(crate) fn case_conversion_return_type(
Copy link
Copy Markdown
Contributor

@neilconway neilconway Mar 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't seem specific to case conversion; we probably need a more generic facility for finding the return return type for a function that takes a string. But we can handle that later.

Copy link
Copy Markdown
Contributor

@Jefffrey Jefffrey Mar 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe we can remove this function entirely in favour of just Ok(arg_types[0].clone()) in the return type implementation; it does mean omitting dictionary handling but it seems the actual invoke code for lower/upper functions don't handle dictionaries anyway?

Copy link
Copy Markdown
Contributor Author

@kumarUjjawal kumarUjjawal Mar 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea.

arg_type: &DataType,
name: &str,
) -> Result<DataType> {
match arg_type {
DataType::Utf8View | DataType::BinaryView => Ok(DataType::Utf8View),
Copy link
Copy Markdown
Contributor

@neilconway neilconway Mar 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe BinaryView will be rejected by the type signatures for these functions, so probably clearer if we don't handle it.

DataType::Dictionary(_, value_type)
if **value_type == DataType::Utf8View
|| **value_type == DataType::BinaryView =>
{
Ok(DataType::Utf8View)
}
_ => utf8_to_str_type(arg_type, name),
}
}

pub(crate) fn to_lower(args: &[ColumnarValue], name: &str) -> Result<ColumnarValue> {
case_conversion(args, |string| string.to_lowercase(), name)
}
Expand Down Expand Up @@ -358,10 +375,8 @@ where
>(array, op)?)),
DataType::Utf8View => {
let string_array = as_string_view_array(array)?;
let mut string_builder = StringBuilder::with_capacity(
string_array.len(),
string_array.get_array_memory_size(),
);
let mut string_builder =
StringViewBuilder::with_capacity(string_array.len());

for str in string_array.iter() {
if let Some(str) = str {
Expand All @@ -386,7 +401,7 @@ where
}
ScalarValue::Utf8View(a) => {
let result = a.as_ref().map(|x| op(x));
Ok(ColumnarValue::Scalar(ScalarValue::Utf8(result)))
Ok(ColumnarValue::Scalar(ScalarValue::Utf8View(result)))
}
other => exec_err!("Unsupported data type {other:?} for function {name}"),
},
Expand Down
37 changes: 31 additions & 6 deletions datafusion/functions/src/string/lower.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,7 @@
use arrow::datatypes::DataType;
use std::any::Any;

use crate::string::common::to_lower;
use crate::utils::utf8_to_str_type;
use crate::string::common::{case_conversion_return_type, to_lower};
use datafusion_common::Result;
use datafusion_common::types::logical_string;
use datafusion_expr::{
Expand Down Expand Up @@ -82,7 +81,7 @@ impl ScalarUDFImpl for LowerFunc {
}

fn return_type(&self, arg_types: &[DataType]) -> Result<DataType> {
utf8_to_str_type(&arg_types[0], "lower")
case_conversion_return_type(&arg_types[0], "lower")
}

fn invoke_with_args(&self, args: ScalarFunctionArgs) -> Result<ColumnarValue> {
Expand All @@ -97,8 +96,7 @@ impl ScalarUDFImpl for LowerFunc {
#[cfg(test)]
mod tests {
use super::*;
use arrow::array::{Array, ArrayRef, StringArray};
use arrow::datatypes::DataType::Utf8;
use arrow::array::{Array, ArrayRef, StringArray, StringViewArray};
use arrow::datatypes::Field;
use datafusion_common::config::ConfigOptions;
use std::sync::Arc;
Expand All @@ -111,7 +109,7 @@ mod tests {
number_rows: input.len(),
args: vec![ColumnarValue::Array(input)],
arg_fields,
return_field: Field::new("f", Utf8, true).into(),
return_field: Field::new("f", expected.data_type().clone(), true).into(),
config_options: Arc::new(ConfigOptions::default()),
};

Expand Down Expand Up @@ -197,4 +195,31 @@ mod tests {

to_lower(input, expected)
}

#[test]
fn lower_utf8view() -> Result<()> {
let input = Arc::new(StringViewArray::from(vec![
Some("ARROW"),
None,
Some("TSCHÜSS"),
])) as ArrayRef;

let expected = Arc::new(StringViewArray::from(vec![
Some("arrow"),
None,
Some("tschüss"),
])) as ArrayRef;

to_lower(input, expected)
}

#[test]
fn lower_return_type_dictionary_utf8view() -> Result<()> {
Copy link
Copy Markdown
Contributor

@neilconway neilconway Mar 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For symmetry, should we add an analogous test case for upper?

let return_type = LowerFunc::new().return_type(&[DataType::Dictionary(
Box::new(DataType::Int32),
Box::new(DataType::Utf8View),
)])?;
assert_eq!(return_type, DataType::Utf8View);
Ok(())
}
}
27 changes: 21 additions & 6 deletions datafusion/functions/src/string/upper.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,7 @@
// specific language governing permissions and limitations
// under the License.

use crate::string::common::to_upper;
use crate::utils::utf8_to_str_type;
use crate::string::common::{case_conversion_return_type, to_upper};
use arrow::datatypes::DataType;
use datafusion_common::Result;
use datafusion_common::types::logical_string;
Expand Down Expand Up @@ -81,7 +80,7 @@ impl ScalarUDFImpl for UpperFunc {
}

fn return_type(&self, arg_types: &[DataType]) -> Result<DataType> {
utf8_to_str_type(&arg_types[0], "upper")
case_conversion_return_type(&arg_types[0], "upper")
}

fn invoke_with_args(&self, args: ScalarFunctionArgs) -> Result<ColumnarValue> {
Expand All @@ -96,8 +95,7 @@ impl ScalarUDFImpl for UpperFunc {
#[cfg(test)]
mod tests {
use super::*;
use arrow::array::{Array, ArrayRef, StringArray};
use arrow::datatypes::DataType::Utf8;
use arrow::array::{Array, ArrayRef, StringArray, StringViewArray};
use arrow::datatypes::Field;
use datafusion_common::config::ConfigOptions;
use std::sync::Arc;
Expand All @@ -110,7 +108,7 @@ mod tests {
number_rows: input.len(),
args: vec![ColumnarValue::Array(input)],
arg_fields: vec![arg_field],
return_field: Field::new("f", Utf8, true).into(),
return_field: Field::new("f", expected.data_type().clone(), true).into(),
config_options: Arc::new(ConfigOptions::default()),
};

Expand Down Expand Up @@ -196,4 +194,21 @@ mod tests {

to_upper(input, expected)
}

#[test]
fn upper_utf8view() -> Result<()> {
let input = Arc::new(StringViewArray::from(vec![
Some("arrow"),
None,
Some("tschüß"),
])) as ArrayRef;

let expected = Arc::new(StringViewArray::from(vec![
Some("ARROW"),
None,
Some("TSCHÜSS"),
])) as ArrayRef;

to_upper(input, expected)
}
}
30 changes: 30 additions & 0 deletions datafusion/sqllogictest/test_files/functions.slt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -445,6 +445,21 @@ SELECT upper(arrow_cast('árvore ação αβγ', 'Dictionary(Int32, Utf8)'))
----
ÁRVORE AÇÃO ΑΒΓ

query T
SELECT arrow_typeof(upper('foo'))
----
Utf8

query T
SELECT arrow_typeof(upper(arrow_cast('foo', 'LargeUtf8')))
----
LargeUtf8

query T
SELECT arrow_typeof(upper(arrow_cast('foo', 'Utf8View')))
Copy link
Copy Markdown
Contributor

@neilconway neilconway Mar 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add SLT tests for dictionary with a Utf8View value type?

Copy link
Copy Markdown
Contributor Author

@kumarUjjawal kumarUjjawal Mar 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure about the new tests i added, can you take a look.

----
Utf8View

query T
SELECT btrim(' foo ')
----
Expand Down Expand Up @@ -500,6 +515,21 @@ SELECT lower(arrow_cast('ÁRVORE AÇÃO ΑΒΓ', 'Dictionary(Int32, Utf8)'))
----
árvore ação αβγ

query T
SELECT arrow_typeof(lower('FOObar'))
----
Utf8

query T
SELECT arrow_typeof(lower(arrow_cast('FOObar', 'LargeUtf8')))
----
LargeUtf8

query T
SELECT arrow_typeof(lower(arrow_cast('FOObar', 'Utf8View')))
----
Utf8View

query T
SELECT ltrim(' foo')
----
Expand Down