Jefffrey commented on code in PR #18674:
URL: https://github.com/apache/datafusion/pull/18674#discussion_r2527733886
##########
datafusion/spark/src/function/conditional/if.rs:
##########
@@ -80,7 +80,8 @@ impl ScalarUDFImpl for SparkIf {
}
fn invoke_with_args(&self, _args: ScalarFunctionArgs) ->
Result<ColumnarValue> {
- internal_err!("if should have been simplified to case")
+ assert_or_internal_err!(false, "if should have been simplified to
case");
+ unreachable!()
Review Comment:
There is little benefit to this refactor if we introduce an `unreachable`
##########
datafusion/spark/src/function/math/hex.rs:
##########
@@ -184,9 +185,7 @@ pub fn compute_hex(
args: &[ColumnarValue],
lowercase: bool,
) -> Result<ColumnarValue, DataFusionError> {
- 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");
Review Comment:
Use `take_function_args`
##########
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:
We should use `take_function_args` here
##########
datafusion/spark/src/function/hash/crc32.rs:
##########
@@ -105,10 +105,12 @@ fn spark_crc32_impl<'a>(input: impl Iterator<Item =
Option<&'a [u8]>>) -> ArrayR
fn spark_crc32(args: &[ArrayRef]) -> Result<ArrayRef> {
let [input] = args else {
- return internal_err!(
- "Spark `crc32` function requires 1 argument, got {}",
- args.len()
+ assert_eq_or_internal_err!(
Review Comment:
I'll refactor this in #18662 anyway
##########
datafusion/spark/src/function/datetime/last_day.rs:
##########
@@ -65,10 +68,12 @@ impl ScalarUDFImpl for SparkLastDay {
fn invoke_with_args(&self, args: ScalarFunctionArgs) ->
Result<ColumnarValue> {
let ScalarFunctionArgs { args, .. } = args;
let [arg] = args.as_slice() else {
- return internal_err!(
- "Spark `last_day` function requires 1 argument, got {}",
- args.len()
+ assert_eq_or_internal_err!(
+ args.len(),
+ 1,
+ "Spark `last_day` function requires 1 argument"
);
+ unreachable!()
Review Comment:
This should be fixed to use `take_function_args` as well
--
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]
