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

fix array_append with null array #8333

Closed
wants to merge 2 commits into from
Closed

fix array_append with null array #8333

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
18de0bf
fix array_append with null array
Veeupup Nov 27, 2023
b0ccf19
fix comments
Veeupup Nov 29, 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
18 changes: 17 additions & 1 deletion datafusion/expr/src/built_in_function.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -536,7 +536,23 @@ impl BuiltinScalarFunction {
let data_type = get_base_type(&input_expr_types[0])?;
Ok(data_type)
}
BuiltinScalarFunction::ArrayAppend => Ok(input_expr_types[0].clone()),
BuiltinScalarFunction::ArrayAppend => {
if let DataType::List(field) = input_expr_types[0].clone() {
if field.data_type().equals_datatype(&DataType::Null) {
Ok(DataType::List(Arc::new(Field::new(
"item",
input_expr_types[1].clone(),
true,
))))
} else {
Ok(input_expr_types[0].clone())
}
} else {
plan_err!(
"The {self} function can only accept list as the first argument"
Veeupup marked this conversation as resolved.
Show resolved Hide resolved
)
}
}
BuiltinScalarFunction::ArrayConcat => {
let mut expr_type = Null;
let mut max_dims = 0;
Expand Down
41 changes: 18 additions & 23 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 @@ -227,16 +227,25 @@ fn compute_array_dims(arr: Option<ArrayRef>) -> Result<Option<Vec<Option<u64>>>>
}

fn check_datatypes(name: &str, args: &[&ArrayRef]) -> Result<()> {
let data_type = args[0].data_type();
if !args.iter().all(|arg| {
arg.data_type().equals_datatype(data_type)
|| arg.data_type().equals_datatype(&DataType::Null)
}) {
let types = args.iter().map(|arg| arg.data_type()).collect::<Vec<_>>();
return plan_err!("{name} received incompatible types: '{types:?}'.");
let mut data_types = args
.iter()
.map(|arg| arg.data_type())
.collect::<HashSet<_>>();
match data_types.len() {
1 => Ok(()),
2 => {
if data_types.remove(&DataType::Null) {
Veeupup marked this conversation as resolved.
Show resolved Hide resolved
Ok(())
} else {
let types = args.iter().map(|arg| arg.data_type()).collect::<Vec<_>>();
Copy link
Contributor

Choose a reason for hiding this comment

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

seems types variable is not used at all?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, my bad, its it err message.
we may want to avoid code duplication for

           let types = args.iter().map(|arg| arg.data_type()).collect::<Vec<_>>();
            plan_err!("{name} received incompatible types: '{types:?}'.")

Copy link
Contributor

Choose a reason for hiding this comment

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

    match x {
        1 => "ok",
        2 if true => "ok",
        _ => "err"
    };

plan_err!("{name} received incompatible types: '{types:?}'.")
}
}
_ => {
let types = args.iter().map(|arg| arg.data_type()).collect::<Vec<_>>();
plan_err!("{name} received incompatible types: '{types:?}'.")
}
}

Ok(())
}

macro_rules! call_array_function {
Expand Down Expand Up @@ -2951,20 +2960,6 @@ mod tests {
assert_eq!(result, &UInt64Array::from_value(2, 1));
}

#[test]
fn test_check_invalid_datatypes() {
let data = vec![Some(vec![Some(1), Some(2), Some(3)])];
let list_array =
Arc::new(ListArray::from_iter_primitive::<Int64Type, _, _>(data)) as ArrayRef;
let int64_array = Arc::new(StringArray::from(vec![Some("string")])) as ArrayRef;

let args = [list_array.clone(), int64_array.clone()];

let array = array_append(&args);

assert_eq!(array.unwrap_err().strip_backtrace(), "Error during planning: array_append received incompatible types: '[Int64, Utf8]'.");
}

fn return_array() -> ArrayRef {
// Returns: [1, 2, 3, 4]
let args = [
Expand Down
16 changes: 8 additions & 8 deletions datafusion/sqllogictest/test_files/array.slt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1056,16 +1056,16 @@ select make_array(['a','b'], null);

# TODO: array_append with NULLs
# array_append scalar function #1
# query ?
# select array_append(make_array(), 4);
# ----
# [4]
query ?
select array_append(make_array(), 4);
----
[4]

# array_append scalar function #2
# query ??
# select array_append(make_array(), make_array()), array_append(make_array(), make_array(4));
# ----
# [[]] [[4]]
query ??
select array_append(make_array(), make_array()), array_append(make_array(), make_array(4));
----
[[]] [[4]]

# array_append scalar function #3
query ???
Expand Down