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

Use take_function_args in more places #14525

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

lgingerich
Copy link

@lgingerich lgingerich commented Feb 6, 2025

Which issue does this PR close?

What changes are included in this PR?

Refactor manual function argument count validation to use the take_function_args function.

Are these changes tested?

All changes are covered by existing tests.

Are there any user-facing changes?

No.

@lgingerich lgingerich marked this pull request as draft February 6, 2025 15:18
@lgingerich
Copy link
Author

@findepi Is there a good way to use take_function_args() outside of the functions crate?

@alamb
Copy link
Contributor

alamb commented Feb 6, 2025

@findepi Is there a good way to use take_function_args() outside of the functions crate?

I think you would have to move it somwehere like datafusion_common

@lgingerich
Copy link
Author

I think you would have to move it somwehere like datafusion_common

Sounds good, I should get to that this weekend and then I'll apply take_function_args() in other crates.

@github-actions github-actions bot added logical-expr Logical plan and expressions common Related to common crate labels Feb 9, 2025
@lgingerich lgingerich marked this pull request as ready for review February 9, 2025 15:52
@alamb alamb changed the title Apply take_function_args Use take_function_args in more places Feb 10, 2025
if arg_types.len() != 1 {
return exec_err!("SUM expects exactly one argument");
}
let [array] = take_function_args(self.name(), arg_types)?;
Copy link
Contributor

Choose a reason for hiding this comment

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

this is so much nicer

@alamb
Copy link
Contributor

alamb commented Feb 10, 2025

FYI @findepi

@alamb
Copy link
Contributor

alamb commented Feb 10, 2025

Thank you so much @lgingerich

I started the CI checks

FYI When I ran some of the tests locally I saw some failures which seem to be related:

 cargo test --test sqllogictests
...
     Running bin/sqllogictests.rs (target/debug/deps/sqllogictests-be41d049b57e846a)
Completed 107 test files in 7 seconds                                                                                       External error: query is expected to fail with error:
	(regex) DataFusion error: Execution error: map requires exactly 2 arguments, got 1 instead
but got error:
	DataFusion error: Execution error: map function requires 2 arguments, got 1
[SQL] SELECT MAP(['POST', 'HEAD'])
at test_files/map.slt:191

Error: Execution("1 failures")
error: test failed, to rerun pass `-p datafusion-sqllogictest --test sqllogictests`

@github-actions github-actions bot added the sqllogictest SQL Logic Tests (.slt) label Feb 10, 2025
@lgingerich
Copy link
Author

@alamb Should be fixed now. I just didn't have the language of the test properly matching this new function.

@lgingerich
Copy link
Author

I'll check out these new errors later today.

Is there a simple way to run all these tests locally without manually running each of the difference cargo test combinations? (I'm on MacOS)

@alamb
Copy link
Contributor

alamb commented Feb 10, 2025

I'll check out these new errors later today.

Is there a simple way to run all these tests locally without manually running each of the difference cargo test combinations? (I'm on MacOS)

not sure -- there may be some scripts here https://github.com/apache/datafusion/tree/main/ci/scripts

It would be great to help make it easier to test locally

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 @lgingerich -- this looks great to me

@alamb
Copy link
Contributor

alamb commented Feb 11, 2025

I took the liberty of merging this PR up from main to resolve some conflicts:

Screenshot 2025-02-11 at 7 10 48 AM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
common Related to common crate functions logical-expr Logical plan and expressions sqllogictest SQL Logic Tests (.slt)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Apply take_function_args to functions validating argument count
2 participants