Skip to content

Conversation

tshauck
Copy link
Contributor

@tshauck tshauck commented Aug 23, 2024

Which issue does this PR close?

Closes #11855

Rationale for this change

The Utf8View type are currently cast into Utf8 types. This PR updates the lower (and upper) functions(s) to support the type natively.

What changes are included in this PR?

Added a specific path for Utf8Views to upper and lower.

Are these changes tested?

Yes, updated the EXPLAIN tests to show no cast is performed. Also basic tests to show the actual transformation is done on the utf8view type.

Are there any user-facing changes?

No

@github-actions github-actions bot added sqllogictest SQL Logic Tests (.slt) functions Changes to functions implementation labels Aug 23, 2024
@tshauck tshauck force-pushed the upper-lower-string-view branch 3 times, most recently from 5737c20 to 5709f3b Compare August 23, 2024 20:02
@tshauck tshauck force-pushed the upper-lower-string-view branch from 5709f3b to b4e0974 Compare August 23, 2024 20:06
_,
>(array, op)?)),
DataType::Utf8View => {
let string_array = as_string_view_array(array)?;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure happy with this implementation, but it seems to work. I tried something akin to:

let result = string_array.iter().map(|string| string.map(op));

but that seems to require a clone of op which isn't allowed based on the existing function definition, and seems worth avoiding.

Copy link
Contributor

Choose a reason for hiding this comment

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

this implemenation looks reasonable to me. Thanks @tshauck

@tshauck tshauck marked this pull request as ready for review August 23, 2024 20:33
Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

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

Looks great to me -- thanks @tshauck

_,
>(array, op)?)),
DataType::Utf8View => {
let string_array = as_string_view_array(array)?;
Copy link
Contributor

Choose a reason for hiding this comment

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

this implemenation looks reasonable to me. Thanks @tshauck

@alamb alamb merged commit e4c3d64 into apache:main Aug 24, 2024
@tshauck tshauck deleted the upper-lower-string-view branch August 24, 2024 15:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

functions Changes to functions implementation sqllogictest SQL Logic Tests (.slt)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Update LOWER scalar function to support Utf8View

2 participants