Skip to content

Conversation

@alamb
Copy link
Contributor

@alamb alamb commented Jun 9, 2023

(Note this looks like a large change but it is actually quite small -- it is just moving code around)

Which issue does this PR close?

N/A
(though inspired by #5781 )

Rationale for this change

This has been a pet peeve of mine and I think it makes working with this code harder than needed as finding important functionality like "what is the signature of this function" harder because they aren't documented on https://docs.rs/datafusion-expr/26.0.0/datafusion_expr/enum.BuiltinScalarFunction.html

What changes are included in this PR?

  1. Move signature, return_type and generate_signature_error_msg to functions on BuiltinScalarFunction
  2. Mark the existing functions as deprecated

Are these changes tested?

existing coverage

Are there any user-facing changes?

Hopefully easier to navigate code

@github-actions github-actions bot added logical-expr Logical plan and expressions optimizer Optimizer rules physical-expr Changes to the physical-expr crates sql SQL Planner labels Jun 9, 2023
@alamb alamb force-pushed the alamb/encapsulate_bulit_in_functions branch from f11a589 to 5bfd5ef Compare June 9, 2023 11:53
.map(|e| e.get_type(schema))
.collect::<Result<Vec<_>>>()?;
function::return_type(fun, &data_types)
fun.return_type(&data_types)
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 a pretty good example of the point of this PR -- the functionality is now part of fun rather than a free function that takes &self as its first argument

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(I hope to apply the same pattern to window_function::return_type and aggregate_function::return_type

Copy link
Member

Choose a reason for hiding this comment

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

(I hope to apply the same pattern to window_function::return_type and aggregate_function::return_type

🚀agree with it

@@ -1,125 +0,0 @@
// Licensed to the Apache Software Foundation (ASF) under one
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 recently added code from @2010YOUY01 (which was awesome) which I simply moved into BuiltInScalarFunction

@alamb alamb marked this pull request as ready for review June 9, 2023 13:20
@jackwener jackwener self-requested a review June 11, 2023 13:12
Copy link
Member

@jackwener jackwener left a comment

Choose a reason for hiding this comment

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

Make sense to me, thanks @alamb .

There are currently some conflicts. After the conflict is resolved, I think it can merge this PR to avoid conflict (the PR of the MOVE behavior is easy to accumulate the conflict)

.map(|e| e.get_type(schema))
.collect::<Result<Vec<_>>>()?;
function::return_type(fun, &data_types)
fun.return_type(&data_types)
Copy link
Member

Choose a reason for hiding this comment

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

(I hope to apply the same pattern to window_function::return_type and aggregate_function::return_type

🚀agree with it

@alamb
Copy link
Contributor Author

alamb commented Jun 22, 2023

(I hope to apply the same pattern to window_function::return_type and aggregate_function::return_type

🚀agree with it

I finally had a chance to whip this out: #6748

@alamb alamb deleted the alamb/encapsulate_bulit_in_functions branch June 22, 2023 17:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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