Reduce code repetition in datafusion/functions mod files#10700
Merged
alamb merged 13 commits intoapache:mainfrom Jun 3, 2024
Merged
Reduce code repetition in datafusion/functions mod files#10700alamb merged 13 commits intoapache:mainfrom
datafusion/functions mod files#10700alamb merged 13 commits intoapache:mainfrom
Conversation
MohamedAbdeen21
commented
May 28, 2024
MohamedAbdeen21
commented
May 28, 2024
datafusion/functions/src/core/mod.rs
Outdated
| ),( | ||
| coalesce, | ||
| "Returns `coalesce(args...)`, which evaluates to the value of the first expr which is not NULL", | ||
| args, |
Contributor
Author
There was a problem hiding this comment.
Single arguments followed by a single comma indicate a vector argument. The only downside here is that rust-analyzer is unable to identify the type of args and the dev is expected to read the doc of export_functions! to notice that args is a vector and not an Expr.
MohamedAbdeen21
commented
May 30, 2024
| ($(($FUNC:ident, $DOC:expr, $($arg:tt)*)),*) => { | ||
| $( | ||
| // switch to single-function cases below | ||
| export_functions!(single $FUNC, $DOC, $($arg)*); |
Contributor
Author
There was a problem hiding this comment.
A little hack to avoid making a separate macro.
MohamedAbdeen21
changed the title
datafusion/functions mod files
alamb
reviewed
May 30, 2024
MohamedAbdeen21
force-pushed
the
reduce-repetition-with-macros
branch
from
4fc8a18 to
15bfd36
Compare
jayzhan211
reviewed
Jun 2, 2024
alamb
reviewed
Jun 3, 2024
Contributor
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
Thanks @jayzhan211 and @MohamedAbdeen21
| )* | ||
| }; | ||
|
|
||
| // single vector argument (a single argument followed by a comma) |
Contributor
Author
|
Thanks for the review! @jayzhan211 @alamb |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Possibly closes #10397.
Rationale for this change
Extending the
export_function!macro to be used in math and core functions modules, reducing code repetition.Some functions expect a single
Exprarg and others expect a singleVec<Expr>args. I took a page from Python's 1-tuples (where(1)is evaluated toint(1), but(1,)is evaluated totuple(1)and now single args followed by a comma are treated as aVec<Expr>but single args without trailing commas are treated asExpr.Variadic functions are unchanged
arg1 arg2 ..., but should not contain commas.I left a comment regarding the special case of
get_field.What changes are included in this PR?
Extending the
export_function!macro to accept both variadic functions and functions that take singleVec<Expr>argument. Uses the rust forums link made by @jayzhan211 https://users.rust-lang.org/t/macro-repetition-with-multiple-rules/110816/2?u=jayzhanAre these changes tested?
Passes existing tests
Are there any user-facing changes?
No