alamb commented on code in PR #9229:
URL: https://github.com/apache/arrow-datafusion/pull/9229#discussion_r1489873260
##########
datafusion/proto/tests/cases/roundtrip_logical_plan.rs:
##########
@@ -932,17 +932,17 @@ fn round_trip_scalar_values() {
ScalarValue::Binary(None),
ScalarValue::LargeBinary(Some(b"bar".to_vec())),
ScalarValue::LargeBinary(None),
- ScalarValue::from((
- vec![
Review Comment:
FWIW this would have panic'd previously if the input was invalid. Using the
`ScalarStructBuilder` makes it clear that this can happen
##########
datafusion/common/src/scalar/mod.rs:
##########
@@ -99,6 +102,66 @@ use arrow_buffer::NullBuffer;
/// # }
/// ```
///
+/// # Nested Types
Review Comment:
It would be nice to have some similar examples for creating `Struct::List`
##########
datafusion/common/src/scalar/mod.rs:
##########
@@ -2710,27 +2774,6 @@ impl From<String> for ScalarValue {
}
}
-// TODO: Remove this after changing to Scalar<T>
Review Comment:
These were added in https://github.com/apache/arrow-datafusion/pull/7893 and
not yet released so this is not a breaking API change
##########
datafusion/common/src/scalar/mod.rs:
##########
@@ -3266,31 +3310,33 @@ mod tests {
),
]);
- let arrays = vec![boolean as ArrayRef, int as ArrayRef];
- let fields = Fields::from(vec![
- Field::new("b", DataType::Boolean, false),
- Field::new("c", DataType::Int32, false),
- ]);
- let sv = ScalarValue::from((fields, arrays));
+ let sv = ScalarStructBuilder::new()
+ .with_array(Field::new("b", DataType::Boolean, false), boolean)
+ .with_array(Field::new("c", DataType::Int32, false), int)
+ .build()
+ .unwrap();
+
let struct_arr = sv.to_array().unwrap();
let actual = as_struct_array(&struct_arr).unwrap();
assert_eq!(actual, &expected);
}
#[test]
- #[should_panic(expected = "assertion `left == right` failed")]
+ #[should_panic(
+ expected = "Error building SclarValue::Struct. Expected array with
exactly one element, found array with 4 elements"
Review Comment:
the builder makes better messages I think
##########
datafusion/common/src/scalar/mod.rs:
##########
@@ -99,6 +102,66 @@ use arrow_buffer::NullBuffer;
/// # }
/// ```
///
+/// # Nested Types
+///
+/// `List` / `LargeList` / `FixedSizeList` / `Struct` are represented as a
+/// single element array of the corresponding type.
+///
+/// ## Example: Creating [`ScalarValue::Struct`] using [`ScalarStructBuilder`]
+/// ```
+/// # use std::sync::Arc;
+/// # use arrow::datatypes::{DataType, Field};
+/// # use datafusion_common::{ScalarValue, scalar::ScalarStructBuilder};
+/// // Build a struct like: {a: 1, b: "foo"}
+/// let field_a = Field::new("a", DataType::Int32, false);
Review Comment:
The use case for the `ScalarStructBuilder` is pretty nicely illustrated by
this example compared to the one in
```
## Example: Creating [`ScalarValue::Struct`] directly
```
--
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]
