Jefffrey commented on PR #18937:
URL: https://github.com/apache/datafusion/pull/18937#issuecomment-3637427815
I took another quick look, and I think some of my prior comments were
inaccurate, I apologize for that. I was mainly thrown by some parts of the code
which lead me down the wrong line of thinking; for example, this signature:
```rust
fn variance_signature() -> Signature {
Signature::one_of(
vec![
TypeSignature::Numeric(1),
TypeSignature::Coercible(vec![Coercion::new_exact(
TypeSignatureClass::Decimal,
)]),
],
Volatility::Immutable,
)
}
```
The coercible part there is redundant since `Numeric(1)` should already
cover decimals; I was mistaken in thinking the new code paths introduced in
this PR were not being in effect because of this misconception.
My high level thoughts on this PR now are:
- Would benefit from cleaning up/refactoring code to be more concise
- For example, the new `is_numeric_or_decimal()` function seems extremely
redundant considering a decimal type is already numeric; having code like this
is quite confusing and makes it harder to understand what's actually important
in this code that we should be reviewing more carefully
- Seems odd there is a manual implementation of casting decimal to float
here (is that what is happening?)
- Is it possible to maintain the return type as decimal instead of casting
to float, or does that introduced precision loss?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
