Minor: Encapsulate return_type and signature in AggregateFunction and WindowFunction
#6748
Conversation
This file contains 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
alamb
force-pushed
the
alamb/encapsulate_aggregate_functions
branch
from
5728dae to
9d0c676
Compare
alamb
commented
Jun 22, 2023
| @@ -246,10 +276,10 @@ mod tests { | |||
| #[test] | |||
| fn test_count_return_type() -> Result<()> { | |||
| let fun = find_df_window_func("count").unwrap(); | |||
| let observed = return_type(&fun, &[DataType::Utf8])?; | |||
| let observed = fun.return_type(&[DataType::Utf8])?; | |||
There was a problem hiding this comment.
This basically shows the new API -- use a method on the fun rather than have to find a free function
github-actions
bot
added
optimizer
alamb
changed the title
return_type and signature in `return_type and signature in AggregateFunction and WindowFunction
alamb
commented
Jun 22, 2023
| &self.signature(), | ||
| )?; | ||
|
|
||
| match self { |
There was a problem hiding this comment.
This code is now indented one level more -- there are no logical changes
jackwener
approved these changes
Jul 1, 2023
|
Thank you for the review @jackwener |
2010YOUY01
pushed a commit
to 2010YOUY01/arrow-datafusion
that referenced
this pull request
Jul 5, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Reviewing the PR with whitespace blind diff I think makes it easier to see what is going on: #6748
This is a very small logical change, but since it changes the indent level the textual change is significant
Which issue does this PR close?
Follow on to #6612
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/window_function/enum.WindowFunction.html or https://docs.rs/datafusion-expr/26.0.0/datafusion_expr/aggregate_function/enum.AggregateFunction.html
What changes are included in this PR?
signature,return_typeto functions onWindowFunctionandBuiltInWindowFunctionandAggregateFunctionAre these changes tested?
existing coverage
Are there any user-facing changes?
Hopefully easier to navigate code
There is no API change as the existing methods still exist, they are just deprecated