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 NullArray to Pass row count to ScalarFunctions that take 0 arguments #328

Merged
merged 2 commits into from
May 16, 2021

Conversation

jimexist
Copy link
Member

Which issue does this PR close?

fix 305 by using a null array as param for zero param functions

Closes #305

Rationale for this change

in case of a zero param function we'll have a rather hacky way to pass in the batch num_rows param so that the functions wrapped can be generative the correct result

What changes are included in this PR?

Similar to #307 but use a null array instead

Are there any user-facing changes?

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.

FWIW I think this looks good, and is the consensus direction for this feature:
(see #303 (comment))

The tests for random() will cover the code in this PR I think.

I had one comment regarding From vs TryFrom but then I think it is ready

datafusion/src/physical_plan/functions.rs Outdated Show resolved Hide resolved
@alamb alamb mentioned this pull request May 14, 2021
@jimexist jimexist force-pushed the fix-305 branch 2 times, most recently from a3c7acc to efcdd4a Compare May 15, 2021 02:52
@codecov-commenter
Copy link

Codecov Report

Merging #328 (fe4a6df) into master (1702d6c) will increase coverage by 0.00%.
The diff coverage is 90.90%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #328   +/-   ##
=======================================
  Coverage   75.72%   75.72%           
=======================================
  Files         143      143           
  Lines       23832    23840    +8     
=======================================
+ Hits        18046    18054    +8     
  Misses       5786     5786           
Impacted Files Coverage Δ
datafusion/src/physical_plan/udf.rs 78.26% <ø> (ø)
datafusion/src/physical_plan/functions.rs 89.60% <90.90%> (+0.05%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1702d6c...fe4a6df. Read the comment docs.

impl BuiltinScalarFunction {
/// an allowlist of functions to take zero arguments, so that they will get special treatment
/// while executing.
fn supports_zero_argument(&self) -> bool {
Copy link
Member Author

Choose a reason for hiding this comment

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

had to add this whitelist because vararg funcs like array will not accept 0 args

Copy link
Member

@jorgecarleitao jorgecarleitao left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks @jimexist !

@alamb alamb changed the title fix 305 by using a null array as param for zero param functions Use NullArray to Pass row count to ScalarFunctions that take 0 arguments May 16, 2021
@alamb alamb merged commit 1c50371 into apache:master May 16, 2021
@alamb
Copy link
Contributor

alamb commented May 16, 2021

I agree -- thanks again @jimexist -- great stuff

@jimexist jimexist deleted the fix-305 branch May 19, 2021 00:19
@houqp houqp added datafusion Changes in the datafusion crate enhancement New feature or request labels Jul 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
datafusion Changes in the datafusion crate enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow execution of zero parameter functions
5 participants