Jefffrey commented on code in PR #18674:
URL: https://github.com/apache/datafusion/pull/18674#discussion_r2525546598
##########
datafusion/spark/src/function/math/modulus.rs:
##########
@@ -87,9 +85,11 @@ impl ScalarUDFImpl for SparkMod {
}
fn return_type(&self, arg_types: &[DataType]) -> Result<DataType> {
- if arg_types.len() != 2 {
- return internal_err!("mod expects exactly two arguments");
- }
+ assert_eq_or_internal_err!(
+ arg_types.len(),
+ 2,
+ "mod expects exactly two arguments"
+ );
Review Comment:
```suggestion
```
We shouldn't be checking this in return type since signature guards this for
us already
##########
datafusion/spark/src/function/datetime/date_add.rs:
##########
@@ -88,10 +90,12 @@ impl ScalarUDFImpl for SparkDateAdd {
fn spark_date_add(args: &[ArrayRef]) -> Result<ArrayRef> {
let [date_arg, days_arg] = args else {
- return internal_err!(
- "Spark `date_add` function requires 2 arguments, got {}",
- args.len()
+ assert_eq_or_internal_err!(
Review Comment:
We should be using `take_function_args` for patterns like this (unpacking
expected no. of arguments), for reference:
https://github.com/apache/datafusion/blob/377c0fce481d561d58e3a6fad2dca18cb1d58384/datafusion/spark/src/function/map/map_from_arrays.rs#L66-L72
##########
datafusion/spark/src/function/datetime/date_sub.rs:
##########
@@ -126,10 +130,12 @@ fn spark_date_sub(args: &[ArrayRef]) -> Result<ArrayRef> {
)?
}
_ => {
- return internal_err!(
- "Spark `date_sub` function: argument must be int8, int16,
int32, got {:?}",
- days_arg.data_type()
- );
+ assert_or_internal_err!(
+ matches!(days_arg.data_type(), DataType::Int8 | DataType::Int16
| DataType::Int32),
+ "Spark `date_sub` function: argument must be int8, int16,
int32, got {:?}",
+ days_arg.data_type()
+ );
+ unreachable!()
Review Comment:
We should keep the original instead of introducing `unreachable!()`
`assert_or_internal_err` should be replacing asserts, not existing
`internal_err`s
##########
datafusion/spark/src/function/math/modulus.rs:
##########
@@ -135,9 +135,11 @@ impl ScalarUDFImpl for SparkPmod {
}
fn return_type(&self, arg_types: &[DataType]) -> Result<DataType> {
- if arg_types.len() != 2 {
- return internal_err!("pmod expects exactly two arguments");
- }
+ assert_eq_or_internal_err!(
+ arg_types.len(),
+ 2,
+ "pmod expects exactly two arguments"
+ );
Review Comment:
```suggestion
```
Ditto
--
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]
