adriangb commented on code in PR #24680:
URL: https://github.com/apache/datafusion/pull/24680#discussion_r3868891922
##########
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:
Can some / all of these be SLT tests instead of or in addition to unit tests?
##########
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:
I wonder if we could reproduce these tests as SLT tests instead of or in
addition to unit tests?
##########
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:
I feel this comment could be improved some. `can leave a Struct cast intact`
is a bit vague. When does this happen? Why?
--
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]
