comphead commented on code in PR #7381:
URL: https://github.com/apache/datafusion-comet/pull/7381#discussion_r4485786036
##########
spark/src/test/resources/sql-tests/expressions/struct/get_struct_field.sql:
##########
@@ -38,3 +38,15 @@ INSERT INTO test_struct_null_field VALUES (1), (-1), (NULL)
query
SELECT s.x, s.n FROM (
SELECT IF(a > 0, named_struct('x', a, 'n', null), null) AS s FROM
test_struct_null_field)
+
+-- A field of a struct that native evaluates as a scalar (a cast of
named_struct over a scalar
+-- subquery) is one value for every row. The table is one file, so at the
default batch size its
+-- four rows share a batch.
+statement
+CREATE TABLE test_struct_scalar(a int) USING parquet
+
+statement
+INSERT INTO test_struct_scalar SELECT /*+ REPARTITION(1) */ * FROM VALUES (1),
(-1), (NULL), (4) AS v(a)
+
+query
+SELECT a, CAST(named_struct('x', (SELECT max(a) FROM test_struct_scalar)) AS
STRUCT<x: BIGINT>).x, CAST(named_struct('x', (SELECT max(a) FROM
test_struct_scalar WHERE a > 100)) AS STRUCT<x: BIGINT>).x FROM
test_struct_scalar
Review Comment:
This only reads an integer field, so the list arms of `single_row_scalar`
and the struct and map arms that go through `try_from_array` are not reached
from SQL. Could the fixture also read an array field, a nested struct field and
a map field of the same kind of cast `named_struct` over the scalar subquery,
for example `CAST(named_struct('l', array((SELECT max(a) FROM
test_struct_scalar))) AS STRUCT<l: ARRAY<BIGINT>>).l`, `named_struct('s',
named_struct('y', <subquery>))` and `named_struct('m', map('k', <subquery>))`?
I expect `CreateNamedStruct` to return a scalar struct for these too since
every field value is a scalar, but I have not run them. A NULL struct would
also be worth a row if `IF(<scalar subquery> > 100, named_struct(...), NULL)`
stays a scalar natively. Today the null-struct case is only covered by the Rust
test.
##########
native/spark-expr/src/struct_funcs/get_struct_field.rs:
##########
@@ -136,10 +158,113 @@ impl Display for GetStructField {
#[cfg(test)]
mod tests {
use super::*;
- use arrow::array::{ArrayRef, Int64Array};
- use arrow::buffer::NullBuffer;
+ use arrow::array::{Int64Array, LargeListViewArray, ListArray,
ListViewArray, NullArray};
+ use arrow::buffer::{NullBuffer, OffsetBuffer, ScalarBuffer};
use arrow::datatypes::Fields;
- use datafusion::physical_expr::expressions::Column;
+ use datafusion::physical_expr::expressions::{Column, Literal};
+
+ // A field of a scalar struct is a scalar, so a consumer broadcasts it
over the batch instead of
+ // reading a one-row array beside full columns. A null struct gives a null
field, for a typed
+ // field and a Null-typed one.
+ #[test]
+ fn field_of_scalar_struct_is_scalar() {
+ let fields: Fields = Fields::from(vec![
+ Field::new("a", DataType::Int64, true),
+ Field::new("n", DataType::Null, true),
+ ]);
+ let schema = Arc::new(Schema::new(vec![Field::new("id",
DataType::Int64, false)]));
+ let batch = RecordBatch::try_new(
+ Arc::clone(&schema),
+ vec![Arc::new(Int64Array::from(vec![0_i64, 1, 2, 3]))],
+ )
+ .unwrap();
+
+ let present = ScalarValue::Struct(Arc::new(StructArray::new(
+ fields.clone(),
+ vec![
+ Arc::new(Int64Array::from(vec![7_i64])),
+ Arc::new(NullArray::new(1)),
+ ],
+ None,
+ )));
+ let absent =
ScalarValue::Struct(Arc::new(StructArray::new_null(fields.clone(), 1)));
+ // A null struct whose child slot still holds a value: the field is
null all the same.
+ let hidden = ScalarValue::Struct(Arc::new(StructArray::new(
+ fields,
+ vec![
+ Arc::new(Int64Array::from(vec![7_i64])),
+ Arc::new(NullArray::new(1)),
+ ],
+ Some(NullBuffer::new_null(1)),
+ )));
+
+ for (scalar, ordinal, expected) in [
+ (present.clone(), 0, ScalarValue::Int64(Some(7))),
+ (present, 1, ScalarValue::Null),
+ (absent.clone(), 0, ScalarValue::Int64(None)),
+ (absent, 1, ScalarValue::Null),
+ (hidden, 0, ScalarValue::Int64(None)),
+ ] {
+ let expr = GetStructField::new(Arc::new(Literal::new(scalar)),
ordinal);
+ match expr.evaluate(&batch).unwrap() {
+ ColumnarValue::Scalar(value) => assert_eq!(value, expected),
+ ColumnarValue::Array(array) => {
+ panic!("expected a scalar for ordinal {ordinal}, got array
{array:?}")
+ }
+ }
+ }
+ }
+
+ // A list field of a scalar struct keeps its element field, metadata
included, so the scalar's
+ // type is the one `data_type` declares.
+ #[test]
+ fn list_field_of_scalar_struct_keeps_its_type() {
+ let item =
+ Arc::new(Field::new("item", DataType::Int64, true).with_metadata(
+ [("ARROW:extension:name".to_string(),
"test.ext".to_string())].into(),
+ ));
+ let values: ArrayRef = Arc::new(Int64Array::from(vec![1_i64, 2]));
+ let lists: Vec<ArrayRef> = vec![
+ Arc::new(ListArray::new(
+ Arc::clone(&item),
+ OffsetBuffer::from_lengths([2]),
+ Arc::clone(&values),
+ None,
+ )),
+ Arc::new(ListViewArray::new(
+ Arc::clone(&item),
+ ScalarBuffer::from(vec![0_i32]),
+ ScalarBuffer::from(vec![2_i32]),
+ Arc::clone(&values),
+ None,
+ )),
+ Arc::new(LargeListViewArray::new(
+ Arc::clone(&item),
+ ScalarBuffer::from(vec![0_i64]),
+ ScalarBuffer::from(vec![2_i64]),
+ Arc::clone(&values),
+ None,
+ )),
+ ];
+ let schema = Schema::new(vec![Field::new("id", DataType::Int64,
false)]);
+ let batch = RecordBatch::try_new(
+ Arc::new(schema.clone()),
+ vec![Arc::new(Int64Array::from(vec![0_i64, 1, 2]))],
+ )
+ .unwrap();
+
+ for list in lists {
+ let fields = Fields::from(vec![Field::new("l",
list.data_type().clone(), true)]);
+ let scalar = ScalarValue::Struct(Arc::new(StructArray::new(fields,
vec![list], None)));
+ let expr = GetStructField::new(Arc::new(Literal::new(scalar)), 0);
+ match expr.evaluate(&batch).unwrap() {
+ ColumnarValue::Scalar(value) => {
+ assert_eq!(value.data_type(),
expr.data_type(&schema).unwrap())
+ }
+ ColumnarValue::Array(array) => panic!("expected a scalar, got
array {array:?}"),
+ }
+ }
Review Comment:
This loop only compares `value.data_type()` for a present list, so it would
still pass if the wrapped list held the wrong row or lost the parent's nulls.
Could it also assert the value (the `[1, 2]` list) and add a null-struct case
for each list width, since that is where `child_with_parent_nulls` feeds the
directly wrapped array?
--
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]
