Skip to content

feat: The "character_length" function handle "Binary" type - #7350

Closed
parkma99 wants to merge 6 commits into
apache:mainfrom
parkma99:fix/character_length
Closed

feat: The "character_length" function handle "Binary" type#7350
parkma99 wants to merge 6 commits into
apache:mainfrom
parkma99:fix/character_length

Conversation

@parkma99

@parkma99parkma99 commented Aug 21, 2023

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Closes#7344

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

@github-actionsgithub-actionsBot added logical-expr Logical plan and expressions physical-expr Changes to the physical-expr crates labels Aug 21, 2023
@JayjeetAtGithub

JayjeetAtGithub commented Aug 21, 2023

Copy link
Copy Markdown
Contributor

Thanks for working on this @parkma99. The fix works great !

@parkma99
parkma99 marked this pull request as ready for review August 22, 2023 12:56
parkma99
parkma99 commented Aug 22, 2023
View reviewed changes
Comment threaddatafusion/physical-expr/src/functions.rs Outdated
let args: Vec<ColumnarValue> = args
.iter()
.map(|col_value| {
cast_column(col_value, &DataType::Utf8, None).unwrap()

@parkma99parkma99Aug 22, 2023

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

In there ,I do not know how to handle this unwrap.

@tustvold

tustvold commented Aug 22, 2023

Copy link
Copy Markdown
Contributor

Perhaps we could implement this as part of the coercion rules as opposed to internal to the evaluation logic? See coerce_arguments_for_fun perhaps?

@parkma99

parkma99 commented Aug 23, 2023

Copy link
Copy Markdown
ContributorAuthor

Perhaps we could implement this as part of the coercion rules as opposed to internal to the evaluation logic? See coerce_arguments_for_fun perhaps?

Thank you, it looks good, I will have a try lately

@parkma99

parkma99 commented Aug 23, 2023

Copy link
Copy Markdown
ContributorAuthor

in #7344 (comment)

But I think it might also be necessary to make the LENGTH function support binaries.

I think it is a good solution. What do you think @tustvold ?

@tustvold

tustvold commented Aug 23, 2023

Copy link
Copy Markdown
Contributor

Responded on the linked ticket

@github-actionsgithub-actionsBot added the optimizer Optimizer rules label Aug 24, 2023
@github-actionsgithub-actionsBot removed the physical-expr Changes to the physical-expr crates label Aug 24, 2023
@parkma99

parkma99 commented Aug 24, 2023

Copy link
Copy Markdown
ContributorAuthor

cc @tustvold@JayjeetAtGithub

alamb
alamb reviewed Aug 24, 2023
View reviewed changes

@alambalamb left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thank you for working on this @parkma99 and @JayjeetAtGithub

Can we please add a .slt level test for this functionality?

I think there is a single coercion rule change (the same as https://github.com/apache/arrow-datafusion/pull/7365/files#r1303360870) that will fix this issue as well

@github-actionsgithub-actionsBot added the sqllogictest SQL Logic Tests (.slt) label Aug 24, 2023
@alambalamb mentioned this pull request Oct 17, 2023
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alambalambalamb left review comments

Assignees

No one assigned

Labels

logical-exprLogical plan and expressionsoptimizerOptimizer rulessqllogictestSQL Logic Tests (.slt)

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Internal error: The "character_length" function can only accept strings

4 participants

@parkma99@JayjeetAtGithub@tustvold@alamb