Fix ApproxPercentileCont signature #8825
Conversation
github-actions
bot
added
logical-expr
| @@ -223,7 +223,7 @@ pub fn coerce_types( | |||
| | AggregateFunction::RegrSXX | |||
| | AggregateFunction::RegrSYY | |||
| | AggregateFunction::RegrSXY => { | |||
| let valid_types = [NUMERICS.to_vec(), vec![DataType::Null]].concat(); | |||
| let valid_types = [NUMERICS.to_vec(), vec![Null]].concat(); | |||
There was a problem hiding this comment.
You didn't do this, but it is unfortunate there are special coercion rules for AggregateFunctions that are not handled by the more general purpose Function cocercion rules.
| @@ -95,6 +95,9 @@ SELECT approx_percentile_cont(c3, 0.95, c1) FROM aggregate_test_100 | |||
| statement error DataFusion error: Error during planning: No function matches the given name and argument types 'APPROX_PERCENTILE_CONT\(Int16, Float64, Float64\)'\. You might need to add explicit type casts\. | |||
| SELECT approx_percentile_cont(c3, 0.95, 111.1) FROM aggregate_test_100 | |||
|
|
|||
| statement error DataFusion error: Error during planning: No function matches the given name and argument types 'APPROX_PERCENTILE_CONT\(Float64, Float64, Float64\)'\. You might need to add explicit type casts\. | |||
There was a problem hiding this comment.
Won't this test pass on main too? Maybe the problem is that the sqllogictest framework doesn't display the hints 🤔
There was a problem hiding this comment.
Yeah that's the issue. Perhaps I should write a unit test instead? 🤔
There was a problem hiding this comment.
If you could that would be great (otherwise I worry we we may break this again during a refactor or something and not realize it)
There was a problem hiding this comment.
Added a unit test 👍
|
@joroKr21 are you willing to add a new test? Or shall we find some help to do so? |
The number of centroids must be an integer in `coerce_types`. Reflect that in the type signature.
joroKr21
force-pushed
the
bugfix/percentile-signature
branch
from
d81defb
to
6b794b5
Compare
The number of centroids must be an integer in
coerce_types. Reflect that in the type signature.Which issue does this PR close?
Didn't create an issue for this one.
Rationale for this change
Currently when we try to pass a floating type as the number of centroids, we get a confusing error message, e.g.
And then later the exact same signature is listed among the candidates.
The reason is that the type signature is not in sync with the logic in
coerce_types.What changes are included in this PR?
Changed the type signature of
ApproxPercentileContto be a combination of all numeric types and all integer types for the number of centroids.Are these changes tested?
I added a test but it's not really feasible to list all possible signatures in the test.
Are there any user-facing changes?
Only in terms of the type signature and error message. No change in behaviour:
(although confusingly the old type signature would include only parameters with types
[t, Float64, t])