Skip to content

Conversation

@jackwener
Copy link
Member

Which issue does this PR close?

Related to #2175 and #3807.

Rationale for this change

Continuing the work outlined in #3807 to bring Expr variants more in line with LogicalPlan variants.

What changes are included in this PR?

Refactors Expr::ScalarFunction to use a struct.

Are there any user-facing changes?

No

@github-actions github-actions bot added core Core DataFusion crate logical-expr Logical plan and expressions optimizer Optimizer rules physical-expr Changes to the physical-expr crates sql SQL Planner labels May 8, 2023
#[derive(Clone, PartialEq, Eq, Hash)]
pub struct ScalarFunction {
/// The function
pub fun: built_in_function::BuiltinScalarFunction,
Copy link
Contributor

Choose a reason for hiding this comment

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

I would love, long term, to remove the difference between ScalarFunction and ScalarUDF (aka have a single way of calling functions for built in and extension functions)

Copy link
Member Author

Choose a reason for hiding this comment

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

It's a great idea, I trace it in #6282

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 like a nice cleanup to me -- thank you @jackwener

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 like a nice cleanup to me -- thank you @jackwener

@alamb alamb added the api change Changes the API exposed to users of the crate label May 8, 2023
@jackwener jackwener merged commit 153356e into apache:main May 8, 2023
@jackwener jackwener deleted the refactor branch May 8, 2023 15:25
@jackwener
Copy link
Member Author

jackwener commented May 8, 2023

Thanks @alamb review.

@jackwener
Copy link
Member Author

Because there is a following PR to refactor ScalarUDF.

So, I merge this PR to continue #6284. (otherwise, it will cause conflict)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api change Changes the API exposed to users of the crate core Core DataFusion crate logical-expr Logical plan and expressions optimizer Optimizer rules physical-expr Changes to the physical-expr crates sql SQL Planner

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants