Skip to content

Conversation

@zjregee
Copy link
Member

@zjregee zjregee commented Feb 25, 2025

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Yes.

Are there any user-facing changes?

None.

@github-actions github-actions bot added logical-expr Logical plan and expressions sqllogictest SQL Logic Tests (.slt) functions Changes to functions implementation labels Feb 25, 2025
@zjregee
Copy link
Member Author

zjregee commented Feb 25, 2025

I think this PR is ready for review now. cc: @jayzhan211

fn btrim<T: OffsetSizeTrait>(args: &[ArrayRef]) -> Result<ArrayRef> {
let use_string_view = args[0].data_type() == &DataType::Utf8View;
general_trim::<T>(args, TrimType::Both, use_string_view)
let args = if args.len() > 1 {
Copy link
Contributor

Choose a reason for hiding this comment

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

casting logic should be in invoke_with_args to keep this kernel function easy to be reuse

Copy link
Contributor

Choose a reason for hiding this comment

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

Now, I think this is fine for private function.

I think we also need to change starts_with to private function in https://github.com/apache/datafusion/pull/14812/files. I don't know why it is public but it shouldn't

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated in #14812.

@jayzhan211 jayzhan211 merged commit f2cdc14 into apache:main Feb 26, 2025
24 checks passed
@jayzhan211
Copy link
Contributor

Thanks @zjregee

@zjregee zjregee deleted the replace-type-signature-for-trim branch February 27, 2025 02:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

functions Changes to functions implementation logical-expr Logical plan and expressions sqllogictest SQL Logic Tests (.slt)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants