chore: Refactor with assert_or_internal_err!() in datafusion/spark. #18674

codetyri0n · 2025-11-13T15:46:17Z

Which issue does this PR close?

Part of #18613

Rationale for this change

What changes are included in this PR?

refactoring conditional checks with assert_or_internal_err!() or assert_eq_or_internal_err!()

Are these changes tested?

Are there any user-facing changes?

codetyri0n · 2025-11-13T16:46:27Z

CC : @2010YOUY01

comphead

Thanks @codetyri0n I'm thinking we need to use exec_err or plan_err instead of internal_err. The last one used for something which is not really expected and classifiable.

martin-g · 2025-11-13T21:22:04Z

datafusion/spark/src/function/datetime/date_add.rs

+        assert_eq_or_internal_err!(
+            args.len(),
+            2,
+            "Spark `date_add` function requires 2 arguments"


Suggested change

"Spark `date_add` function requires 2 arguments"

"Spark `date_add` function requires 2 arguments, got {}"

This is useful while debugging. Please keep it!

martin-g · 2025-11-13T21:29:23Z

datafusion/spark/src/function/datetime/date_add.rs

-        return internal_err!(
-            "Spark `date_add` function requires 2 arguments, got {}",
-            args.len()
+        assert_eq_or_internal_err!(


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

We should be using take_function_args for patterns like this (unpacking expected no. of arguments), for reference:

datafusion/datafusion/spark/src/function/map/map_from_arrays.rs

Lines 66 to 72 in 377c0fc

fn return_type(&self, arg_types: &[DataType]) -> Result<DataType> {

let [key_type, value_type] = take_function_args("map_from_arrays", arg_types)?;

Ok(map_type_from_key_value_types(

get_element_type(key_type)?,

get_element_type(value_type)?,

))

}

martin-g · 2025-11-13T21:34:07Z

datafusion/spark/src/function/datetime/make_interval.rs

+                assert_or_internal_err!(
+                    matches!(
+                        other,
+                        ColumnarValue::Array(_)


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

martin-g · 2025-11-13T21:34:56Z

datafusion/spark/src/function/datetime/make_interval.rs

+                    matches!(
+                        other,
+                        ColumnarValue::Array(_)
+                            | ColumnarValue::Scalar(ScalarValue::IntervalMonthDayNano(_))


Suggested change

| ColumnarValue::Scalar(ScalarValue::IntervalMonthDayNano(_))

| 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

martin-g · 2025-11-13T21:35:46Z

datafusion/spark/src/function/hash/sha1.rs

-        return internal_err!(
-            "Spark `sha1` function requires 1 argument, got {}",
-            args.len()
+        assert_eq_or_internal_err!(


Same as above - not really an improvement

martin-g · 2025-11-13T21:36:05Z

datafusion/spark/src/function/math/factorial.rs

-    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");


We should use take_function_args here

Jefffrey · 2025-11-14T02:22:56Z

datafusion/spark/src/function/datetime/date_add.rs

-        return internal_err!(
-            "Spark `date_add` function requires 2 arguments, got {}",
-            args.len()
+        assert_eq_or_internal_err!(


We should be using take_function_args for patterns like this (unpacking expected no. of arguments), for reference:

datafusion/datafusion/spark/src/function/map/map_from_arrays.rs

Lines 66 to 72 in 377c0fc

fn return_type(&self, arg_types: &[DataType]) -> Result<DataType> {

let [key_type, value_type] = take_function_args("map_from_arrays", arg_types)?;

Ok(map_type_from_key_value_types(

get_element_type(key_type)?,

get_element_type(value_type)?,

))

}

Jefffrey · 2025-11-14T02:23:42Z

datafusion/spark/src/function/datetime/date_sub.rs

+            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!()


We should keep the original instead of introducing unreachable!()

assert_or_internal_err should be replacing asserts, not existing internal_errs

datafusion/spark/src/function/math/modulus.rs

codetyri0n · 2025-11-14T10:14:54Z

thanks @Jefffrey @martin-g
I would like to make sure I get my understanding right (for this and future PRs)- please let me know if otherwise :
We would need the refactoring only for the function signature for improvements for the errors with better detailing user for example -
SELECT date_add('2025-01-10');
and it should be returning something like -
Spark date_add function requires 2 arguments, got 1

realised that using this macro is not an improvement for return types and etc.

codetyri0n · 2025-11-14T13:46:11Z

datafusion/spark/src/function/datetime/date_add.rs

-            args.len()
-        );
-    };
+    let [date_arg, days_arg] = take_function_args("date_add", args)?;


Handled this with take_function_args as suggested. Noticed a few functions that have not used this, thinking ill include those in a separate PR.

Jefffrey

Most of these refactors should use take_function_args as it is much more ergonomic and handles the error for us anyway

Jefffrey · 2025-11-14T14:35:28Z

datafusion/spark/src/function/conditional/if.rs

-        internal_err!("if should have been simplified to case")
+        assert_or_internal_err!(false, "if should have been simplified to case");
+        unreachable!()


There is little benefit to this refactor if we introduce an unreachable

yup, my apologies I forgot to revert this change (and the one below as well 🤦🏽 ). : )

Jefffrey · 2025-11-14T14:35:52Z

datafusion/spark/src/function/datetime/last_day.rs

+            assert_eq_or_internal_err!(
+                args.len(),
+                1,
+                "Spark `last_day` function requires 1 argument"
            );
+            unreachable!()


This should be fixed to use take_function_args as well

Jefffrey · 2025-11-14T14:36:22Z

datafusion/spark/src/function/math/factorial.rs

-    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");


We should use take_function_args here

Jefffrey · 2025-11-14T14:37:34Z

datafusion/spark/src/function/hash/crc32.rs

-        return internal_err!(
-            "Spark `crc32` function requires 1 argument, got {}",
-            args.len()
+        assert_eq_or_internal_err!(


I'll refactor this in #18662 anyway

Jefffrey · 2025-11-14T14:37:56Z

datafusion/spark/src/function/math/hex.rs

-    if args.len() != 1 {
-        return internal_err!("hex expects exactly one argument");
-    }
+    assert_eq_or_internal_err!(args.len(), 1, "hex expects exactly one argument");


Use take_function_args

codetyri0n · 2025-11-14T19:13:08Z

this is good to go (CI failure due to No space left on device)

chore: Refactor with assert_or_internal_err!() in datafusion/spark.

18440ea

github-actions bot added the spark label Nov 13, 2025

codetyri0n added 2 commits November 13, 2025 21:38

refactor cont...

790f1d0

Clippy fix

fcd703b

comphead reviewed Nov 13, 2025

View reviewed changes

martin-g reviewed Nov 13, 2025

View reviewed changes

Jefffrey reviewed Nov 14, 2025

View reviewed changes

Revert unnecessary refactoring and add improvement for rint.

0942d8e

codetyri0n commented Nov 14, 2025

View reviewed changes

Jefffrey mentioned this pull request Nov 14, 2025

[Tracking] Refactor with assert_or_internal_err!() across the codebase #18613

Closed

14 tasks

Jefffrey reviewed Nov 14, 2025

View reviewed changes

Use take_function_args

487c9e2

Jefffrey approved these changes Nov 15, 2025

View reviewed changes

Merge branch 'main' into refactor_spark_err

f106fee

Jefffrey added this pull request to the merge queue Nov 15, 2025

Merged via the queue into apache:main with commit 17050a9 Nov 15, 2025
28 checks passed

codetyri0n deleted the refactor_spark_err branch November 19, 2025 17:50

	"Spark `date_add` function requires 2 arguments"
	"Spark `date_add` function requires 2 arguments, got {}"

	fn return_type(&self, arg_types: &[DataType]) -> Result<DataType> {
	let [key_type, value_type] = take_function_args("map_from_arrays", arg_types)?;
	Ok(map_type_from_key_value_types(
	get_element_type(key_type)?,
	get_element_type(value_type)?,
	))
	}

	\| ColumnarValue::Scalar(ScalarValue::IntervalMonthDayNano(_))
	\| ColumnarValue::Scalar(ScalarValue::IntervalMonthDayNano(None))

chore: Refactor with assert_or_internal_err!() in datafusion/spark. #18674

chore: Refactor with assert_or_internal_err!() in datafusion/spark. #18674

Uh oh!

Conversation

codetyri0n commented Nov 13, 2025

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

codetyri0n commented Nov 13, 2025

Uh oh!

comphead left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

codetyri0n commented Nov 14, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Jefffrey left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codetyri0n Nov 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codetyri0n commented Nov 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codetyri0n Nov 14, 2025 •

edited

Loading

codetyri0n commented Nov 14, 2025 •

edited

Loading