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

Minor: Remove unnecessary name field in ScalarFunctionDefintion #8365

Merged
merged 1 commit into from
Nov 30, 2023

Conversation

alamb
Copy link
Contributor

@alamb alamb commented Nov 29, 2023

Which issue does this PR close?

Related to / follow on to #8258 from @2010YOUY01

Rationale for this change

While integrating #8258 into our downstream system, I found creating a BuildInScalarFunction to be quite awkward as it had a seemingly duplicated name field. I thought the name field was to handle aliases or something, but it turns out I could remove it easily.

What changes are included in this PR?

Basically change the ScalarFunctionDefinition::BuiltIn to match the others

    BuiltIn {
        fun: built_in_function::BuiltinScalarFunction,
        name: Arc<str>,
    },

to

    BuiltIn(BuiltinScalarFunction),

Are these changes tested?

Covered by existing tests

Are there any user-facing changes?

While this is technically an API change the API hasn't been released so it isn't an additional breaking API change

@github-actions github-actions bot added sql SQL Planner logical-expr Logical plan and expressions physical-expr Physical Expressions optimizer Optimizer rules core Core DataFusion crate labels Nov 29, 2023
fun: built_in_function::BuiltinScalarFunction,
name: Arc<str>,
},
BuiltIn(BuiltinScalarFunction),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the core change here -- don't keep another string around, but just use the function on demand

Copy link
Member

@viirya viirya left a comment

Choose a reason for hiding this comment

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

Looks more clear to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Core DataFusion crate logical-expr Logical plan and expressions optimizer Optimizer rules physical-expr Physical Expressions sql SQL Planner
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants