Skip to content

Change error type of invalid argument to PlanError rather than InternalError, remove misleading comments #7355

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

Merged
alamb merged 5 commits into from
Aug 23, 2023
Merged

Change error type of invalid argument to PlanError rather than InternalError, remove misleading comments #7355

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
5b15971
Fix error type of invalid argument types, remove misleading comments
alamb Aug 21, 2023
4387eae
tests for arrays
alamb Aug 21, 2023
ed5cf49
more tests
alamb Aug 21, 2023
ac7bc40
Add more tests
alamb Aug 21, 2023
ffe2626
fix error
alamb Aug 21, 2023
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
25 changes: 8 additions & 17 deletions datafusion/expr/src/built_in_function.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -525,7 +525,7 @@ impl BuiltinScalarFunction {
}
}
_ => {
return internal_err!(
return plan_err!(
"The {self} function can only accept list as the args."
)
}
Expand All @@ -542,7 +542,7 @@ impl BuiltinScalarFunction {
}
BuiltinScalarFunction::ArrayElement => match &input_expr_types[0] {
List(field) => Ok(field.data_type().clone()),
_ => internal_err!(
_ => plan_err!(
"The {self} function can only accept list as the first argument"
),
},
Expand Down Expand Up @@ -608,7 +608,7 @@ impl BuiltinScalarFunction {
Timestamp(Microsecond, _) => Ok(Timestamp(Microsecond, None)),
Timestamp(Millisecond, _) => Ok(Timestamp(Millisecond, None)),
Timestamp(Second, _) => Ok(Timestamp(Second, None)),
_ => internal_err!(
_ => plan_err!(
"The {self} function can only accept timestamp as the second arg."
),
}
Expand Down Expand Up @@ -677,8 +677,7 @@ impl BuiltinScalarFunction {
LargeBinary => LargeUtf8,
Null => Null,
_ => {
// this error is internal as `data_types` should have captured this.
return internal_err!(
return plan_err!(
"The encode function can only accept utf8 or binary."
);
}
Expand All @@ -690,8 +689,7 @@ impl BuiltinScalarFunction {
LargeBinary => LargeBinary,
Null => Null,
_ => {
// this error is internal as `data_types` should have captured this.
return internal_err!(
return plan_err!(
"The decode function can only accept utf8 or binary."
);
}
Expand All @@ -709,10 +707,7 @@ impl BuiltinScalarFunction {
BuiltinScalarFunction::ToHex => Ok(match input_expr_types[0] {
Int8 | Int16 | Int32 | Int64 => Utf8,
_ => {
// this error is internal as `data_types` should have captured this.
return internal_err!(
"The to_hex function can only accept integers."
);
return plan_err!("The to_hex function can only accept integers.");
}
}),
BuiltinScalarFunction::ToTimestamp => Ok(Timestamp(Nanosecond, None)),
Expand All @@ -737,8 +732,7 @@ impl BuiltinScalarFunction {
Utf8 => List(Arc::new(Field::new("item", Utf8, true))),
Null => Null,
_ => {
// this error is internal as `data_types` should have captured this.
return internal_err!(
return plan_err!(
Copy link
Contributor Author

@alamb alamb Aug 21, 2023

Choose a reason for hiding this comment

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

it is true that this is currently caught by higher up type checks, but I think it is better to throw a plan error here to tell users about the problem if something changes rather than throw a confusing internal error

Also given that this code is often copy/pasted it probably should default to returning a nice error message rather than an opaque one

"The regexp_extract function can only accept strings."
);
}
Expand Down Expand Up @@ -1400,7 +1394,6 @@ macro_rules! make_utf8_to_return_type {
DataType::Utf8 => $utf8Type,
DataType::Null => DataType::Null,
_ => {
// this error is internal as `data_types` should have captured this.
return plan_err!(
"The {:?} function can only accept strings, but got {:?}.",
name,
Expand All @@ -1409,7 +1402,6 @@ macro_rules! make_utf8_to_return_type {
}
},
data_type => {
// this error is internal as `data_types` should have captured this.
return plan_err!(
"The {:?} function can only accept strings, but got {:?}.",
name,
Expand All @@ -1432,8 +1424,7 @@ fn utf8_or_binary_to_binary_type(arg_type: &DataType, name: &str) -> Result<Data
| DataType::LargeBinary => DataType::Binary,
DataType::Null => DataType::Null,
_ => {
// this error is internal as `data_types` should have captured this.
return internal_err!(
return plan_err!(
"The {name:?} function can only accept strings or binary arrays."
);
}
Expand Down
14 changes: 14 additions & 0 deletions datafusion/sqllogictest/test_files/array.slt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -600,6 +600,11 @@ from arrays_values_without_nulls;

## array_element (aliases: array_extract, list_extract, list_element)

# array_element error
query error DataFusion error: Error during planning: The array_element function can only accept list as the first argument
select array_element(1, 2);


# array_element scalar function #1 (with positive index)
query IT
select array_element(make_array(1, 2, 3, 4, 5), 2), array_element(make_array('h', 'e', 'l', 'l', 'o'), 3);
Expand Down Expand Up @@ -1098,6 +1103,10 @@ select array_repeat([1], column3), array_repeat(column1, 3) from arrays_values_w

## array_concat (aliases: `array_cat`, `list_concat`, `list_cat`)

# array_concat error
query error DataFusion error: Error during planning: The array_concat function can only accept list as the args\.
select array_concat(1, 2);

# array_concat scalar function #1
query ??
select array_concat(make_array(1, 2, 3), make_array(4, 5, 6), make_array(7, 8, 9)), array_concat(make_array([1], [2]), make_array([3], [4]));
Expand Down Expand Up @@ -2008,6 +2017,11 @@ NULL 10

## array_dims (aliases: `list_dims`)

# array dims error
# TODO this is a separate bug
query error Internal error: could not cast value to arrow_array::array::list_array::GenericListArray<i32>\.
select array_dims(1);

# array_dims scalar function
query ???
select array_dims(make_array(1, 2, 3)), array_dims(make_array([1, 2], [3, 4])), array_dims(make_array([[[[1], [2]]]]));
Expand Down
25 changes: 25 additions & 0 deletions datafusion/sqllogictest/test_files/encoding.slt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,24 @@ CREATE TABLE test(
(2, NULL, NULL, NULL)
;

# errors
query error DataFusion error: Error during planning: The encode function can only accept utf8 or binary\.
select encode(12, 'hex')

query error DataFusion error: Error during planning: There is no built\-in encoding named 'non_encoding', currently supported encodings are: base64, hex
select encode(bin_field, 'non_encoding') from test;

query error DataFusion error: Error during planning: The decode function can only accept utf8 or binary\.
select decode(12, 'hex')

query error DataFusion error: Error during planning: There is no built\-in encoding named 'non_encoding', currently supported encodings are: base64, hex
select decode(hex_field, 'non_encoding') from test;

query error DataFusion error: SQL error: ParserError\("Expected an SQL statement, found: Candidate"\)
Candidate functions:
to_hex\(Int64\)
select to_hex(hex_field) from test;

# Arrays tests
query T
SELECT encode(bin_field, 'hex') FROM test ORDER BY num;
Expand All @@ -48,3 +66,10 @@ SELECT arrow_cast(decode(hex_field, 'hex'), 'Utf8') FROM test ORDER BY num;
abc
qweqwe
NULL

query T
select to_hex(num) from test ORDER BY num;
----
0
1
2
4 changes: 4 additions & 0 deletions datafusion/sqllogictest/test_files/predicates.slt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,6 +192,10 @@ statement ok
CREATE TABLE IF NOT EXISTS test AS VALUES('foo'),('Barrr'),('Bazzz'),('ZZZZZ');

# async fn test_regexp_is_match
query error Error during planning: Cannot infer common argument type for regex operation Int64 \~ Utf8
SELECT * FROM test WHERE 12 ~ 'z'


query T
SELECT * FROM test WHERE column1 ~ 'z'
----
Expand Down
4 changes: 4 additions & 0 deletions datafusion/sqllogictest/test_files/timestamps.slt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -390,6 +390,10 @@ select to_timestamp_seconds(cast (1 as int));
## test date_bin function
##########

# invalid second arg type
query error DataFusion error: Error during planning: No function matches the given name and argument types 'date_bin\(Interval\(MonthDayNano\), Int64, Timestamp\(Nanosecond, None\)\)'\.
SELECT DATE_BIN(INTERVAL '0 second', 25, TIMESTAMP '1970-01-01T00:00:00Z')

# not support interval 0
statement error Execution error: DATE_BIN stride must be non-zero
SELECT DATE_BIN(INTERVAL '0 second', TIMESTAMP '2022-08-03 14:38:50.000000006Z', TIMESTAMP '1970-01-01T00:00:00Z')
Expand Down