Skip to content

Commit

Permalink
fix bugs
Browse files Browse the repository at this point in the history
  • Loading branch information
Lordworms committed Feb 2, 2025
1 parent 68e372f commit edde88d
Show file tree
Hide file tree
Showing 5 changed files with 94 additions and 9 deletions.
7 changes: 5 additions & 2 deletions datafusion/expr-common/src/type_coercion/binary.rs
Original file line number Diff line number Diff line change
Expand Up @@ -614,7 +614,11 @@ pub fn try_type_union_resolution_with_struct(
let mut keys_string: Option<String> = None;
for data_type in data_types {
if let DataType::Struct(fields) = data_type {
let keys = fields.iter().map(|f| f.name().to_owned()).join(",");
let keys = fields
.iter()
.map(|f| f.name().to_owned())
.sorted()
.join(",");
if let Some(ref k) = keys_string {
if *k != keys {
return exec_err!("Expect same keys for struct type but got mismatched pair {} and {}", *k, keys);
Expand Down Expand Up @@ -671,7 +675,6 @@ pub fn try_type_union_resolution_with_struct(
}
final_struct_types.push(DataType::Struct(new_fields.into()))
}

Ok(final_struct_types)
}

Expand Down
12 changes: 10 additions & 2 deletions datafusion/expr/src/logical_plan/plan.rs
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ use datafusion_common::{
FunctionalDependencies, ParamValues, Result, ScalarValue, TableReference,
UnnestOptions,
};
use datafusion_expr_common::type_coercion::binary::try_type_union_resolution_with_struct;
use indexmap::IndexSet;

// backwards compatibility
Expand Down Expand Up @@ -2695,7 +2696,15 @@ impl Union {
// TODO apply type coercion here, or document why it's better to defer
// temporarily use the data type from the left input and later rely on the analyzer to
// coerce the two schemas into a common one.
first_field.data_type()
let first_type = first_field.data_type();
if matches!(first_type, DataType::Struct(_)) {
let types: Vec<DataType> =
fields.iter().map(|f| f.data_type().clone()).collect();
&try_type_union_resolution_with_struct(&types)
.map(|types| types[0].clone())?
} else {
first_type
}
} else {
fields.iter().skip(1).try_fold(
first_field.data_type(),
Expand Down Expand Up @@ -2728,7 +2737,6 @@ impl Union {
// Functional Dependencies doesn't preserve after UNION operation
let schema = DFSchema::new_with_metadata(union_fields, union_schema_metadata)?;
let schema = Arc::new(schema);

Ok(schema)
}
}
Expand Down
19 changes: 15 additions & 4 deletions datafusion/optimizer/src/analyzer/type_coercion.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
use std::sync::Arc;

use datafusion_expr::binary::BinaryTypeCoercer;
use datafusion_expr::binary::{try_type_union_resolution_with_struct, BinaryTypeCoercer};
use itertools::izip;

use arrow::datatypes::{DataType, Field, IntervalUnit, Schema};
Expand Down Expand Up @@ -843,7 +843,7 @@ fn coerce_case_expression(case: Case, schema: &DFSchema) -> Result<Case> {
.as_ref()
.map(|expr| expr.get_type(schema))
.transpose()?;
let then_types = case
let mut then_types = case
.when_then_expr
.iter()
.map(|(_when, then)| then.get_type(schema))
Expand All @@ -853,7 +853,6 @@ fn coerce_case_expression(case: Case, schema: &DFSchema) -> Result<Case> {
.as_ref()
.map(|expr| expr.get_type(schema))
.transpose()?;

// find common coercible types
let case_when_coerce_type = case_type
.as_ref()
Expand All @@ -873,6 +872,19 @@ fn coerce_case_expression(case: Case, schema: &DFSchema) -> Result<Case> {
})
})
.transpose()?;
// do checks
if then_types.iter().any(|t| matches!(t, DataType::Struct(_)))
|| else_type
.as_ref()
.is_some_and(|t| matches!(t, DataType::Struct(_)))
{
if let Some(ref else_t) = else_type {
then_types.push(else_t.clone());
try_type_union_resolution_with_struct(&then_types)?;
then_types.pop();
}
}

let then_else_coerce_type =
get_coerce_type_for_case_expression(&then_types, else_type.as_ref()).ok_or_else(
|| {
Expand All @@ -882,7 +894,6 @@ fn coerce_case_expression(case: Case, schema: &DFSchema) -> Result<Case> {
)
},
)?;

// do cast if found common coercible types
let case_expr = case
.expr
Expand Down
41 changes: 40 additions & 1 deletion datafusion/sqllogictest/test_files/case.slt
Original file line number Diff line number Diff line change
Expand Up @@ -408,13 +408,52 @@ FROM t;
{xxx: c, foo: d}

# coerce structs with subset of fields
query error Failed to coerce then
query error
SELECT
case
when column1 > 0 then column3
else column4
end
FROM t;
----

DataFusion error: type_coercion
caused by
Execution error: Expect same keys for struct type but got mismatched pair foo,xxx and xxx


statement ok
drop table t

statement ok
create table t as values
(
{ 'foo': 'baz' },
{ 'xxx': arrow_cast('blarg', 'Utf8View') }
);

query error
select CASE WHEN 1=2 THEN column1 ELSE column2 END from t ;
----
DataFusion error: type_coercion
caused by
Execution error: Expect same keys for struct type but got mismatched pair foo and xxx


statement ok
drop table t

statement ok
create table t as values
(
{ 'name': 'Alice', 'age': 25 },
{ 'age': 30, 'name': 'Bob' }
);

query ?
select CASE WHEN 1=2 THEN column1 ELSE column2 END from t;
----
{age: 30, name: Bob}

statement ok
drop table t
24 changes: 24 additions & 0 deletions datafusion/sqllogictest/test_files/union.slt
Original file line number Diff line number Diff line change
Expand Up @@ -851,3 +851,27 @@ FROM (
----
NULL false
foo true

statement ok
drop table t

statement ok
create table t as values
(
{ 'foo': 'baz' },
{ 'xxx': arrow_cast('blarg', 'Utf8View') }
);

query error DataFusion error: Execution error: Expect same keys for struct type but got mismatched pair foo and xxx
select column1 from t UNION ALL select column2 from t;

query ?
SELECT { 'name': 'Alice', 'age': 20 }
UNION ALL
SELECT { 'age': 30, 'name': 'Bob' };
----
{name: 30, age: Bob}
{name: Alice, age: 20}

statement ok
drop table t

0 comments on commit edde88d

Please sign in to comment.