Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Minor: Fix error messages in array expressions #8781

Merged
merged 2 commits into from
Jan 8, 2024
Merged

Minor: Fix error messages in array expressions #8781

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3d111b8
Fix error messages in array expressions
Weijun-H Jan 8, 2024
eaf96bc
fix fmt
Weijun-H Jan 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 16 additions & 20 deletions datafusion/physical-expr/src/array_expressions.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -529,7 +529,7 @@ fn general_except<OffsetSize: OffsetSizeTrait>(

pub fn array_except(args: &[ArrayRef]) -> Result<ArrayRef> {
if args.len() != 2 {
return internal_err!("array_except needs two arguments");
return exec_err!("array_except needs two arguments");
Copy link
Contributor

@jayzhan211 jayzhan211 Jan 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When to use internal_err, when to use exec_err?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

internal_err: something that wasn't expected/anticipated by the implementation and that is most likely a bug (the error message even encourages users to open a bug report). I user should not be able to trigger this under normal circumstances. Note that I/O errors (or any error that happens due to external systems) do NOT fall under this category (there's an external error variant for that)

exec_err: a processing error that happens during execution, e.g. the user passed malformed arguments to a SQL method, opened a CSV file that is broken, tried to divide an integer by zero. these errors shouldn't happen for a "good" query + "good" data, but since both the query and the data can easily be broken or misaligned, these are common occurrences in ETL systems / databases.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is such a great explanation I figured I would encode it in the comments of DataFusionError in its own PR: #8792

}

let array1 = &args[0];
Expand Down Expand Up @@ -894,7 +894,7 @@ pub fn gen_range(args: &[ArrayRef]) -> Result<ArrayRef> {
as_int64_array(&args[1])?,
Some(as_int64_array(&args[2])?),
),
_ => return internal_err!("gen_range expects 1 to 3 arguments"),
_ => return exec_err!("gen_range expects 1 to 3 arguments"),
};

let mut values = vec![];
Expand Down Expand Up @@ -948,7 +948,7 @@ pub fn array_sort(args: &[ArrayRef]) -> Result<ArrayRef> {
nulls_first: order_nulls_first(nulls_first)?,
})
}
_ => return internal_err!("array_sort expects 1 to 3 arguments"),
_ => return exec_err!("array_sort expects 1 to 3 arguments"),
};

let list_array = as_list_array(&args[0])?;
Expand Down Expand Up @@ -994,15 +994,15 @@ fn order_desc(modifier: &str) -> Result<bool> {
match modifier.to_uppercase().as_str() {
"DESC" => Ok(true),
"ASC" => Ok(false),
_ => internal_err!("the second parameter of array_sort expects DESC or ASC"),
_ => exec_err!("the second parameter of array_sort expects DESC or ASC"),
}
}

fn order_nulls_first(modifier: &str) -> Result<bool> {
match modifier.to_uppercase().as_str() {
"NULLS FIRST" => Ok(true),
"NULLS LAST" => Ok(false),
_ => internal_err!(
_ => exec_err!(
"the third parameter of array_sort expects NULLS FIRST or NULLS LAST"
),
}
Expand Down Expand Up @@ -1208,7 +1208,7 @@ pub fn array_empty(args: &[ArrayRef]) -> Result<ArrayRef> {
match array_type {
DataType::List(_) => array_empty_dispatch::<i32>(&args[0]),
DataType::LargeList(_) => array_empty_dispatch::<i64>(&args[0]),
_ => internal_err!("array_empty does not support type '{array_type:?}'."),
_ => exec_err!("array_empty does not support type '{array_type:?}'."),
}
}

Expand Down Expand Up @@ -1598,7 +1598,9 @@ fn array_remove_internal(
let list_array = array.as_list::<i64>();
general_remove::<i64>(list_array, element_array, arr_n)
}
_ => internal_err!("array_remove_all expects a list array"),
array_type => {
exec_err!("array_remove_all does not support type '{array_type:?}'.")
}
}
}

Expand Down Expand Up @@ -2260,10 +2262,7 @@ pub fn array_length(args: &[ArrayRef]) -> Result<ArrayRef> {
match &args[0].data_type() {
DataType::List(_) => array_length_dispatch::<i32>(args),
DataType::LargeList(_) => array_length_dispatch::<i64>(args),
_ => internal_err!(
"array_length does not support type '{:?}'",
args[0].data_type()
),
array_type => exec_err!("array_length does not support type '{array_type:?}'"),
}
}

Expand All @@ -2288,11 +2287,8 @@ pub fn array_dims(args: &[ArrayRef]) -> Result<ArrayRef> {
.map(compute_array_dims)
.collect::<Result<Vec<_>>>()?
}
_ => {
return exec_err!(
"array_dims does not support type '{:?}'",
args[0].data_type()
);
array_type => {
return exec_err!("array_dims does not support type '{array_type:?}'");
}
};

Expand Down Expand Up @@ -2441,7 +2437,7 @@ pub fn array_has_any(args: &[ArrayRef]) -> Result<ArrayRef> {
DataType::LargeList(_) => {
general_array_has_dispatch::<i64>(&args[0], &args[1], ComparisonType::Any)
}
_ => internal_err!("array_has_any does not support type '{array_type:?}'."),
_ => exec_err!("array_has_any does not support type '{array_type:?}'."),
}
}

Expand All @@ -2460,7 +2456,7 @@ pub fn array_has_all(args: &[ArrayRef]) -> Result<ArrayRef> {
DataType::LargeList(_) => {
general_array_has_dispatch::<i64>(&args[0], &args[1], ComparisonType::All)
}
_ => internal_err!("array_has_all does not support type '{array_type:?}'."),
_ => exec_err!("array_has_all does not support type '{array_type:?}'."),
}
}

Expand Down Expand Up @@ -2543,7 +2539,7 @@ pub fn string_to_array<T: OffsetSizeTrait>(args: &[ArrayRef]) -> Result<ArrayRef
});
}
_ => {
return internal_err!(
return exec_err!(
"Expect string_to_array function to take two or three parameters"
)
}
Expand Down Expand Up @@ -2611,7 +2607,7 @@ pub fn array_distinct(args: &[ArrayRef]) -> Result<ArrayRef> {
let array = as_large_list_array(&args[0])?;
general_array_distinct(array, field)
}
_ => internal_err!("array_distinct only support list array"),
array_type => exec_err!("array_distinct does not support type '{array_type:?}'"),
}
}

Expand Down