martin-g commented on code in PR #18674:
URL: https://github.com/apache/datafusion/pull/18674#discussion_r2525004820
##########
datafusion/spark/src/function/datetime/make_interval.rs:
##########
@@ -533,36 +536,41 @@ mod tests {
.ok_or_else(|| {
internal_datafusion_err!("expected
IntervalMonthDayNanoArray")
})?;
- if arr.len() != number_rows {
- return internal_err!(
- "expected array length {number_rows}, got {}",
- arr.len()
- );
- }
+ assert_eq_or_internal_err!(
+ arr.len(),
+ number_rows,
+ "expected array length {number_rows}, got {}",
+ arr.len()
+ );
for i in 0..number_rows {
let iv = arr.value(i);
- if (iv.months, iv.days, iv.nanoseconds) != (0, 0, 0) {
- return internal_err!(
- "row {i}: expected (0,0,0), got ({},{},{})",
- iv.months,
- iv.days,
- iv.nanoseconds
- );
- }
- }
- }
- ColumnarValue::Scalar(ScalarValue::IntervalMonthDayNano(Some(iv)))
=> {
- if (iv.months, iv.days, iv.nanoseconds) != (0, 0, 0) {
- return internal_err!(
- "expected scalar 0s, got ({},{},{})",
+ assert_eq_or_internal_err!(
+ (iv.months, iv.days, iv.nanoseconds),
+ (0, 0, 0),
+ "row {i}: expected (0,0,0), got ({},{},{})",
iv.months,
iv.days,
iv.nanoseconds
);
}
}
+ ColumnarValue::Scalar(ScalarValue::IntervalMonthDayNano(Some(iv)))
=> {
+ assert_eq_or_internal_err!(
+ (iv.months, iv.days, iv.nanoseconds),
+ (0, 0, 0),
+ "expected scalar 0s, got ({},{},{})",
+ iv.months,
+ iv.days,
+ iv.nanoseconds
+ );
+ }
other => {
- return internal_err!(
+ assert_or_internal_err!(
+ matches!(
+ other,
+ ColumnarValue::Array(_)
Review Comment:
What is the idea here ?
ColumnarValue::Array(arr) is already handled at
https://github.com/codetyri0n/datafusion/blob/fcd703b784d3ec4d9976c47a55b52d22c0c08757/datafusion/spark/src/function/datetime/make_interval.rs#L532
##########
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:
IMO this change does not improve it. We are already sure that the args
length is not 2, so there is no need to check it again.
Also now this requires using `unreachable!()` at line 98
##########
datafusion/spark/src/function/datetime/make_interval.rs:
##########
@@ -533,36 +536,41 @@ mod tests {
.ok_or_else(|| {
internal_datafusion_err!("expected
IntervalMonthDayNanoArray")
})?;
- if arr.len() != number_rows {
- return internal_err!(
- "expected array length {number_rows}, got {}",
- arr.len()
- );
- }
+ assert_eq_or_internal_err!(
+ arr.len(),
+ number_rows,
+ "expected array length {number_rows}, got {}",
+ arr.len()
+ );
for i in 0..number_rows {
let iv = arr.value(i);
- if (iv.months, iv.days, iv.nanoseconds) != (0, 0, 0) {
- return internal_err!(
- "row {i}: expected (0,0,0), got ({},{},{})",
- iv.months,
- iv.days,
- iv.nanoseconds
- );
- }
- }
- }
- ColumnarValue::Scalar(ScalarValue::IntervalMonthDayNano(Some(iv)))
=> {
- if (iv.months, iv.days, iv.nanoseconds) != (0, 0, 0) {
- return internal_err!(
- "expected scalar 0s, got ({},{},{})",
+ assert_eq_or_internal_err!(
+ (iv.months, iv.days, iv.nanoseconds),
+ (0, 0, 0),
+ "row {i}: expected (0,0,0), got ({},{},{})",
iv.months,
iv.days,
iv.nanoseconds
);
}
}
+ ColumnarValue::Scalar(ScalarValue::IntervalMonthDayNano(Some(iv)))
=> {
+ assert_eq_or_internal_err!(
+ (iv.months, iv.days, iv.nanoseconds),
+ (0, 0, 0),
+ "expected scalar 0s, got ({},{},{})",
+ iv.months,
+ iv.days,
+ iv.nanoseconds
+ );
+ }
other => {
- return internal_err!(
+ assert_or_internal_err!(
+ matches!(
+ other,
+ ColumnarValue::Array(_)
+ |
ColumnarValue::Scalar(ScalarValue::IntervalMonthDayNano(_))
Review Comment:
```suggestion
|
ColumnarValue::Scalar(ScalarValue::IntervalMonthDayNano(None))
```
?!
Because `Some(iv)` is also already handled at
https://github.com/codetyri0n/datafusion/blob/fcd703b784d3ec4d9976c47a55b52d22c0c08757/datafusion/spark/src/function/datetime/make_interval.rs#L557
##########
datafusion/spark/src/function/math/factorial.rs:
##########
@@ -99,9 +101,7 @@ const FACTORIALS: [i64; 21] = [
];
pub fn spark_factorial(args: &[ColumnarValue]) -> Result<ColumnarValue,
DataFusionError> {
- if args.len() != 1 {
- return internal_err!("`factorial` expects exactly one argument");
- }
+ assert_eq_or_internal_err!(args.len(), 1, "`factorial` expects exactly one
argument");
Review Comment:
👍🏻
##########
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!(
+ args.len(),
+ 2,
+ "Spark `date_add` function requires 2 arguments"
Review Comment:
```suggestion
"Spark `date_add` function requires 2 arguments, got {}"
```
This is useful while debugging. Please keep it!
##########
datafusion/spark/src/function/hash/sha1.rs:
##########
@@ -117,10 +117,12 @@ fn spark_sha1_impl<'a>(input: impl Iterator<Item =
Option<&'a [u8]>>) -> ArrayRe
fn spark_sha1(args: &[ArrayRef]) -> Result<ArrayRef> {
let [input] = args else {
- return internal_err!(
- "Spark `sha1` function requires 1 argument, got {}",
- args.len()
+ assert_eq_or_internal_err!(
Review Comment:
Same as above - not really an improvement
--
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]
