Skip to content
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.

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

Move bit_length and chr functions to datafusion_functions #9782

Merged
merged 5 commits into from
Mar 25, 2024

Conversation

PsiACE
Copy link
Member

@PsiACE PsiACE commented Mar 24, 2024

Which issue does this PR close?

Closes #9534.

Rationale for this change

As part of #9285 the bit_length and chr string functions should be migrated to the new datafusion-functions crate in the new structure

What changes are included in this PR?

Move bit_length and chr functions to datafusion_functions

Are these changes tested?

yes

Are there any user-facing changes?

No

@github-actions github-actions bot added logical-expr Logical plan and expressions physical-expr Physical Expressions sqllogictest SQL Logic Tests (.slt) core Core DataFusion crate labels Mar 24, 2024
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.

Thank you for the contribution @PsiACE -- I think the only thing needed for this PR is to fix the expr_fn parameter signature.

🚀

@@ -111,7 +111,7 @@ async fn test_fn_ascii() -> Result<()> {

#[tokio::test]
async fn test_fn_bit_length() -> Result<()> {
let expr = bit_length(col("a"));
let expr = bit_length(vec![col("a")]);
Copy link
Contributor

Choose a reason for hiding this comment

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

since bit_length only ever takes a single parameter, I think it would be better to keep this as a single expr argument (to keep the UX nicer to call bit_length).

If the function can take potentially different numbers of parameters, that is when Vec<Expr> is more appropriate I think

Copy link
Member Author

Choose a reason for hiding this comment

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

datafusion/functions/src/string/mod.rs Outdated Show resolved Hide resolved
datafusion/functions/src/string/mod.rs Outdated Show resolved Hide resolved
datafusion/sqllogictest/test_files/expr.slt Show resolved Hide resolved
@github-actions github-actions bot removed the core Core DataFusion crate label Mar 25, 2024
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 -- thank you @PsiACE . Very nicely done

@alamb alamb merged commit f7e5581 into apache:main Mar 25, 2024
23 checks passed
@PsiACE PsiACE deleted the bitlength-chr branch March 26, 2024 11:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
logical-expr Logical plan and expressions physical-expr Physical Expressions sqllogictest SQL Logic Tests (.slt)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move ascii, bit_length, btrim, chr to datafusion-functions
2 participants