Minor: add the concise way for matching numerics #5537
Conversation
| other => Err(DataFusionError::Plan(format!( | ||
| "VAR does not support {other:?}" | ||
| ))), | ||
| if NUMERICS.contains(&arg_type) { |
There was a problem hiding this comment.
🤔 this is very close to https://docs.rs/arrow/34.0.0/arrow/datatypes/enum.DataType.html#method.is_numeric except that is_numeric also includes Decimals.
There was a problem hiding this comment.
I also verified that NUMERICS contains the same values
https://github.com/apache/arrow-datafusion/blob/8c34ca4fa34787b137b48ce4f6ffd41b64a1a633/datafusion/expr/src/type_coercion/aggregates.rs#L30-L41
| other => Err(DataFusionError::Plan(format!( | ||
| "VAR does not support {other:?}" | ||
| ))), | ||
| if NUMERICS.contains(arg_type) { |
There was a problem hiding this comment.
Probably we can reuse Datatype::is_numeric()
There was a problem hiding this comment.
I think the DataType::is_numeric also includes Decimals 🤔
There was a problem hiding this comment.
Right, perhaps we can do a helper method checking is_numeric and exclude decimals.
There was a problem hiding this comment.
sort of
fn is_numeric(arg_type: &DataType) -> bool {
return arg_type.is_numeric() && match arg_type {
Decimal128(_, _) | Decimal() => false
_ => true
}
}
|
Thanks everyone! |
|
Benchmark runs are scheduled for baseline = 8c34ca4 and contender = ac5676f. ac5676f is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
Rationale for this change
The array "NUMERICS", that are defined in the file "datafusion/expr/src/type_coercion/aggregates.rs" (see https://github.com/apache/arrow-datafusion/blob/main/datafusion/expr/src/type_coercion/aggregates.rs#L30-L41) contains all types, which found in many "match" and "matches!" cases.
I think this PR suggests more concise way for these cases.
What changes are included in this PR?
Rewritten "match" statements and "matches!" macros.
Are these changes tested?
Yes
Are there any user-facing changes?
No