sunchao commented on code in PR #24680:
URL: https://github.com/apache/datafusion/pull/24680#discussion_r3875936414
##########
datafusion/physical-expr-adapter/src/schema_rewriter.rs:
##########
@@ -1623,36 +1753,492 @@ mod tests {
(logical, physical)
}
+ fn decimal_cast_leaf_types(data_type: DataType) -> Vec<DataType> {
+ let item = Arc::new(Field::new("item", data_type.clone(), true));
+ vec![
+ data_type.clone(),
+ DataType::List(Arc::clone(&item)),
+ DataType::LargeList(Arc::clone(&item)),
+ DataType::FixedSizeList(Arc::clone(&item), 2),
+ DataType::ListView(Arc::clone(&item)),
+ DataType::LargeListView(item),
+ DataType::Map(
+ Arc::new(Field::new(
+ "entries",
+ DataType::Struct(
+ vec![
+ Field::new("key", DataType::Utf8, false),
+ Field::new("value", data_type.clone(), true),
+ ]
+ .into(),
+ ),
+ false,
+ )),
+ false,
+ ),
+ DataType::Dictionary(Box::new(DataType::Int8),
Box::new(data_type.clone())),
+ DataType::new_list(
+ DataType::Struct(
+ vec![Field::new("value", data_type.clone(), true)].into(),
+ ),
+ true,
+ ),
+ DataType::new_list(DataType::new_list(data_type, true), true),
+ ]
+ }
+
+ #[test]
+ fn test_narrow_struct_cast_preserves_struct_unwrapping() -> Result<()> {
+ use arrow::array::{
+ ArrayRef, Decimal128Array, DictionaryArray, Int8Array, ListArray,
+ };
+ use arrow::buffer::OffsetBuffer;
+ use arrow::datatypes::Int8Type;
+
+ let values = Arc::new(StructArray::new(
+ vec![Field::new("value", DataType::Int32, true)].into(),
+ vec![Arc::new(Int32Array::from(vec![1]))],
+ None,
+ )) as ArrayRef;
+ let dictionary = Arc::new(DictionaryArray::<Int8Type>::try_new(
+ Int8Array::from(vec![0]),
+ values,
+ )?) as ArrayRef;
+ let expected_struct = Arc::new(StructArray::new(
+ vec![Field::new("value", DataType::Decimal128(10, 2),
true)].into(),
+ vec![Arc::new(
+ Decimal128Array::from(vec![100]).with_precision_and_scale(10,
2)?,
+ )],
+ None,
+ )) as ArrayRef;
+ let wrap_list = |values: ArrayRef| -> ArrayRef {
+ Arc::new(ListArray::new(
+ Arc::new(Field::new("item", values.data_type().clone(), true)),
+ OffsetBuffer::from_lengths([1]),
+ values,
+ None,
+ ))
+ };
+ let wrap_dictionary = |values: ArrayRef| -> ArrayRef {
+ Arc::new(
+ DictionaryArray::<Int8Type>::try_new(Int8Array::from(vec![0]),
values)
+ .unwrap(),
+ )
+ };
+ for (label, physical, expected) in [
+ (
+ "direct Dictionary",
+ Arc::clone(&dictionary),
+ Arc::clone(&expected_struct),
+ ),
+ (
+ "List of Dictionary",
+ wrap_list(Arc::clone(&dictionary)),
+ wrap_list(Arc::clone(&expected_struct)),
+ ),
+ (
+ "Dictionary of Dictionary",
+ wrap_dictionary(dictionary),
+ wrap_dictionary(expected_struct),
+ ),
+ ] {
+ let (logical_schema, physical_schema) = struct_schemas(
+ vec![Field::new("x", physical.data_type().clone(), true)],
+ vec![Field::new("x", expected.data_type().clone(), true)],
+ );
+ let DataType::Struct(fields) =
physical_schema.field(0).data_type() else {
+ unreachable!()
+ };
+ let batch = RecordBatch::try_new(
+ Arc::clone(&physical_schema),
+ vec![Arc::new(StructArray::new(
+ fields.clone(),
+ vec![physical],
+ None,
+ ))],
+ )?;
+ let adapter = DefaultPhysicalExprAdapterFactory
+ .create(Arc::clone(&logical_schema), physical_schema)?;
+ let rewritten = adapter.rewrite(get_field_expr(&logical_schema,
"s", "x"))?;
+ let actual = rewritten.evaluate(&batch)?.into_array(1)?;
+ assert_eq!(actual.to_data(), expected.to_data(), "{label}");
+ }
+ Ok(())
+ }
+
/// `s['x']` where the file stores `x` as `Int32` and the table declares
/// `Int64` must cast the extracted field, not the whole struct, so that
/// the column stays visible under the `get_field`.
///
/// See <https://github.com/apache/datafusion/issues/24109>.
#[test]
fn test_narrow_struct_cast_to_field_access() {
- let (logical_schema, physical_schema) = struct_schemas(
- vec![Field::new("x", DataType::Int32, true)],
- vec![Field::new("x", DataType::Int64, true)],
- );
+ for (physical_type, logical_type) in [
+ (DataType::Int32, DataType::Int64),
+ (
+ DataType::new_list(DataType::Int32, true),
+ DataType::new_list(DataType::Int64, true),
+ ),
+ ] {
+ let (logical_schema, physical_schema) = struct_schemas(
+ vec![Field::new("x", physical_type.clone(), true)],
+ vec![Field::new("x", logical_type.clone(), true)],
+ );
+
+ let adapter = DefaultPhysicalExprAdapterFactory
+ .create(Arc::clone(&logical_schema), physical_schema)
+ .unwrap();
+ let rewritten = adapter
+ .rewrite(get_field_expr(&logical_schema, "s", "x"))
+ .unwrap();
+
+ let cast = assert_cast_expr(&rewritten);
+ assert_eq!(cast.cast_type(), &logical_type);
+ let get_field = cast
+ .expr()
+ .downcast_ref::<ScalarFunctionExpr>()
+ .expect("Expected get_field under the cast");
+ assert_eq!(get_field.return_type(), &physical_type);
+ assert!(
+ get_field.args()[0].downcast_ref::<Column>().is_some(),
+ "the struct column must not be hidden behind a cast, got:
{rewritten}"
+ );
+ }
+ }
- let adapter = DefaultPhysicalExprAdapterFactory
- .create(Arc::clone(&logical_schema), physical_schema)
- .unwrap();
- let rewritten = adapter
- .rewrite(get_field_expr(&logical_schema, "s", "x"))
- .unwrap();
+ /// Selecting one field of an explicit cast must still evaluate sibling
+ /// conversions, even when schema adaptation inserts another cast below it.
+ #[test]
+ fn test_narrow_struct_cast_preserves_explicit_cast_errors() -> Result<()> {
Review Comment:
Added SQL coverage in `schema_evolution_nested.slt` in
[ec4b3f282](https://github.com/apache/datafusion/pull/24680/commits/ec4b3f282cfb82693cd1fac89eced5f7f5a2dd14):
all-null scalar and List decimal schema evolution, plus an explicit Struct
cast whose unselected `y` conversion must still fail.
I kept the Rust tests as well. They directly distinguish adapter-generated
and pre-existing casts and check logical Field metadata/nullability, expression
shape, and Arrow container cases that SQL alone does not cover. The
explicit-cast SQL case is a useful integration control; the direct adapter test
remains the reproduction of the provenance bug.
##########
datafusion/datasource-parquet/src/row_filter.rs:
##########
@@ -1294,6 +1300,68 @@ mod test {
candidate.read_plan.projection_mask, expected_mask,
"projection_mask should select only the accessed struct field leaf"
);
+
+ // Schema adaptation can leave a Struct cast intact. Its runtime filter
+ // must read every sibling, while planning still rejects explicit
casts.
Review Comment:
Clarified in
[ec4b3f282](https://github.com/apache/datafusion/pull/24680/commits/ec4b3f282cfb82693cd1fac89eced5f7f5a2dd14).
Schema adaptation retains Struct ancestors around some decimal conversions so
an entirely null parent can skip converting its children.
The comment now also explains why this test must still convert `label`: it
is named by the cast target, even though `get_field` selects `value`. The
extended fixture has an unused third sibling, which is pruned while the
required `label` conversion still fails.
##########
datafusion/core/tests/parquet/expr_adapter.rs:
##########
@@ -790,6 +790,212 @@ async fn
test_physical_expr_adapter_with_non_null_defaults() {
assert_batches_eq!(expected, &batches);
}
+#[tokio::test]
+async fn test_explicit_struct_cast_projection_preserves_sibling_errors() ->
Result<()> {
Review Comment:
Added these as Parquet-backed SQL cases in `schema_evolution_nested.slt` in
[ec4b3f282](https://github.com/apache/datafusion/pull/24680/commits/ec4b3f282cfb82693cd1fac89eced5f7f5a2dd14).
The fixtures are generated with `COPY` and reopened with evolved schemas. They
cover the explicit sibling-cast error and scalar/List decimal conversions
beneath an entirely null Struct, including null predicates with filter pushdown
disabled and enabled. Statistics and pruning shortcuts are disabled for the
filter cases, and the settings are reset afterward.
I retained the Rust integration tests for their additional physical-plan and
adapter assertions; the SLTs now cover the user-visible SQL behavior alongside
them.
--
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]
