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

Open
kumarUjjawal wants to merge 3 commits into
base: main
Choose a base branch
from
+103 −18
Open

Make lower and upper emit Utf8View for Utf8View input #20616

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 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
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
74 changes: 58 additions & 16 deletions datafusion/functions/src/string/common.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ use std::sync::Arc;
use crate::strings::make_and_append_view;
use arrow::array::{
Array, ArrayRef, GenericStringArray, GenericStringBuilder, NullBufferBuilder,
OffsetSizeTrait, StringBuilder, StringViewArray, new_null_array,
OffsetSizeTrait, StringBuilder, StringViewArray, StringViewBuilder, new_null_array,
};
use arrow::buffer::{Buffer, ScalarBuffer};
use arrow::datatypes::DataType;
Expand Down Expand Up @@ -332,17 +332,34 @@ fn string_trim<T: OffsetSizeTrait, Tr: Trimmer>(args: &[ArrayRef]) -> Result<Arr
}

pub(crate) fn to_lower(args: &[ColumnarValue], name: &str) -> Result<ColumnarValue> {
case_conversion(args, |string| string.to_lowercase(), name)
case_conversion(
args,
|string| string.to_lowercase(),
name,
Utf8ViewOutput::Utf8View,
)
}

pub(crate) fn to_upper(args: &[ColumnarValue], name: &str) -> Result<ColumnarValue> {
case_conversion(args, |string| string.to_uppercase(), name)
case_conversion(
args,
|string| string.to_uppercase(),
name,
Utf8ViewOutput::Utf8,
)
}

#[derive(Debug, Clone, Copy)]
enum Utf8ViewOutput {
Copy link
Contributor

@neilconway neilconway Feb 28, 2026

Choose a reason for hiding this comment

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

Why do we need a new enum -- can't we just pass the return data type the caller expects?

Utf8,
Utf8View,
}

fn case_conversion<'a, F>(
args: &'a [ColumnarValue],
op: F,
name: &str,
utf8view_output: Utf8ViewOutput,
) -> Result<ColumnarValue>
where
F: Fn(&'a str) -> String,
Expand All @@ -358,20 +375,38 @@ 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(),
);

for str in string_array.iter() {
if let Some(str) = str {
string_builder.append_value(op(str));
} else {
string_builder.append_null();
match utf8view_output {
Utf8ViewOutput::Utf8 => {
Copy link
Contributor

@neilconway neilconway Feb 28, 2026

Choose a reason for hiding this comment

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

I'm confused why we need to add code to handle the "Utf8View input, Utf8 output" case -- seems like we don't actually want that behavior. If we fixed up upper and lower as part of the same PR, couldn't we just have case_conversion return a value of the same type as its input? That would avoid this redundant code and also get rid of the utf8view_output parameter.

let mut string_builder = StringBuilder::with_capacity(
string_array.len(),
string_array.get_array_memory_size(),
);

for str in string_array.iter() {
if let Some(str) = str {
string_builder.append_value(op(str));
} else {
string_builder.append_null();
}
}

Ok(ColumnarValue::Array(Arc::new(string_builder.finish())))
}
Utf8ViewOutput::Utf8View => {
let mut string_builder =
StringViewBuilder::with_capacity(string_array.len());

for str in string_array.iter() {
if let Some(str) = str {
string_builder.append_value(op(str));
} else {
string_builder.append_null();
}
}

Ok(ColumnarValue::Array(Arc::new(string_builder.finish())))
}
}

Ok(ColumnarValue::Array(Arc::new(string_builder.finish())))
}
other => exec_err!("Unsupported data type {other:?} for function {name}"),
},
Expand All @@ -386,7 +421,14 @@ where
}
ScalarValue::Utf8View(a) => {
let result = a.as_ref().map(|x| op(x));
Ok(ColumnarValue::Scalar(ScalarValue::Utf8(result)))
match utf8view_output {
Utf8ViewOutput::Utf8 => {
Ok(ColumnarValue::Scalar(ScalarValue::Utf8(result)))
}
Utf8ViewOutput::Utf8View => {
Ok(ColumnarValue::Scalar(ScalarValue::Utf8View(result)))
}
}
}
other => exec_err!("Unsupported data type {other:?} for function {name}"),
},
Expand Down
28 changes: 24 additions & 4 deletions datafusion/functions/src/string/lower.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,11 @@ impl ScalarUDFImpl for LowerFunc {
}

fn return_type(&self, arg_types: &[DataType]) -> Result<DataType> {
utf8_to_str_type(&arg_types[0], "lower")
if arg_types[0] == DataType::Utf8View {
Ok(DataType::Utf8View)
} else {
utf8_to_str_type(&arg_types[0], "lower")
Copy link
Contributor

@neilconway neilconway Feb 28, 2026

Choose a reason for hiding this comment

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

I wonder if it would be helpful to have a helper here that handles all three string representations and returns the optimal return type. Having a helper for Utf8 vs. LargeUtf8 but handling Utf8View at the call-site seems a bit odd.

}
}

fn invoke_with_args(&self, args: ScalarFunctionArgs) -> Result<ColumnarValue> {
Expand All @@ -97,8 +101,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 +114,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 +200,21 @@ 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)
}
}
15 changes: 15 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 @@ -500,6 +500,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