You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "jayzhan211 (via GitHub)" <gi...@apache.org> on 2023/10/21 03:23:57 UTC
[PR] Change ScalarValue::Struct to ArrayRef [arrow-datafusion]
jayzhan211 opened a new pull request, #7893:
URL: https://github.com/apache/arrow-datafusion/pull/7893
## Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
-->
Closes #7835
## Rationale for this change
<!--
Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.
-->
## What changes are included in this PR?
<!--
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
-->
## Are these changes tested?
<!--
We typically require tests for all PRs in order to:
1. Prevent the code from being accidentally broken by subsequent changes
2. Serve as another way to document the expected behavior of the code
If tests are not included in your PR, please explain why (for example, are they covered by existing tests)?
-->
## Are there any user-facing changes?
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
<!--
If there are any breaking changes to public APIs, please add the `api change` label.
-->
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Change ScalarValue::Struct to ArrayRef [arrow-datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #7893:
URL: https://github.com/apache/arrow-datafusion/pull/7893#discussion_r1449363796
##########
datafusion/physical-expr/src/aggregate/array_agg_ordered.rs:
##########
@@ -308,18 +319,30 @@ impl OrderSensitiveArrayAggAccumulator {
let fields = ordering_fields(&self.ordering_req, &self.datatypes[1..]);
let struct_field = Fields::from(fields.clone());
- let orderings: Vec<ScalarValue> = self
- .ordering_values
- .iter()
- .map(|ordering| {
- ScalarValue::Struct(Some(ordering.clone()), struct_field.clone())
- })
- .collect();
- let struct_type = DataType::Struct(Fields::from(fields));
+ let mut column_wise_ordering_values = vec![];
+ let num_columns = fields.len();
+ for i in 0..num_columns {
Review Comment:
cc @mustafasrepo
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Change ScalarValue::Struct to ArrayRef [arrow-datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on code in PR #7893:
URL: https://github.com/apache/arrow-datafusion/pull/7893#discussion_r1447405114
##########
datafusion/common/src/scalar.rs:
##########
@@ -2695,14 +2730,22 @@ impl From<String> for ScalarValue {
}
}
-impl From<Vec<(&str, ScalarValue)>> for ScalarValue {
- fn from(value: Vec<(&str, ScalarValue)>) -> Self {
- let (fields, scalars): (SchemaBuilder, Vec<_>) = value
- .into_iter()
- .map(|(name, scalar)| (Field::new(name, scalar.data_type(), false), scalar))
- .unzip();
+// Wrapper for ScalarValue::Struct that checks the length of the arrays, without nulls
+impl From<(Fields, Vec<ArrayRef>)> for ScalarValue {
+ fn from((fields, arrays): (Fields, Vec<ArrayRef>)) -> Self {
+ Self::from((fields, arrays, None))
+ }
+}
- Self::Struct(Some(scalars), fields.finish().fields)
+// Wrapper for ScalarValue::Struct that checks the length of the arrays
+impl From<(Fields, Vec<ArrayRef>, Option<NullBuffer>)> for ScalarValue {
Review Comment:
We dont need this after changing Struct with `Scalar<T>`
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Change ScalarValue::Struct to ArrayRef [arrow-datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #7893:
URL: https://github.com/apache/arrow-datafusion/pull/7893#issuecomment-1924748492
@jayzhan211 -- is this PR ready for a review?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Change ScalarValue::Struct to ArrayRef [arrow-datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on code in PR #7893:
URL: https://github.com/apache/arrow-datafusion/pull/7893#discussion_r1447446341
##########
datafusion/common/src/scalar.rs:
##########
@@ -2695,14 +2730,22 @@ impl From<String> for ScalarValue {
}
}
-impl From<Vec<(&str, ScalarValue)>> for ScalarValue {
- fn from(value: Vec<(&str, ScalarValue)>) -> Self {
- let (fields, scalars): (SchemaBuilder, Vec<_>) = value
- .into_iter()
- .map(|(name, scalar)| (Field::new(name, scalar.data_type(), false), scalar))
- .unzip();
+// Wrapper for ScalarValue::Struct that checks the length of the arrays, without nulls
Review Comment:
TODO: Remove these two.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Change ScalarValue::Struct to ArrayRef [arrow-datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on PR #7893:
URL: https://github.com/apache/arrow-datafusion/pull/7893#issuecomment-1829829125
wait on #7862
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Change ScalarValue::Struct to ArrayRef [arrow-datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on code in PR #7893:
URL: https://github.com/apache/arrow-datafusion/pull/7893#discussion_r1447405114
##########
datafusion/common/src/scalar.rs:
##########
@@ -2695,14 +2730,22 @@ impl From<String> for ScalarValue {
}
}
-impl From<Vec<(&str, ScalarValue)>> for ScalarValue {
- fn from(value: Vec<(&str, ScalarValue)>) -> Self {
- let (fields, scalars): (SchemaBuilder, Vec<_>) = value
- .into_iter()
- .map(|(name, scalar)| (Field::new(name, scalar.data_type(), false), scalar))
- .unzip();
+// Wrapper for ScalarValue::Struct that checks the length of the arrays, without nulls
+impl From<(Fields, Vec<ArrayRef>)> for ScalarValue {
+ fn from((fields, arrays): (Fields, Vec<ArrayRef>)) -> Self {
+ Self::from((fields, arrays, None))
+ }
+}
- Self::Struct(Some(scalars), fields.finish().fields)
+// Wrapper for ScalarValue::Struct that checks the length of the arrays
+impl From<(Fields, Vec<ArrayRef>, Option<NullBuffer>)> for ScalarValue {
Review Comment:
We dont need this after changing Struct with Scalar<T>
##########
datafusion/common/src/scalar.rs:
##########
@@ -2695,14 +2730,22 @@ impl From<String> for ScalarValue {
}
}
-impl From<Vec<(&str, ScalarValue)>> for ScalarValue {
- fn from(value: Vec<(&str, ScalarValue)>) -> Self {
- let (fields, scalars): (SchemaBuilder, Vec<_>) = value
- .into_iter()
- .map(|(name, scalar)| (Field::new(name, scalar.data_type(), false), scalar))
- .unzip();
+// Wrapper for ScalarValue::Struct that checks the length of the arrays, without nulls
+impl From<(Fields, Vec<ArrayRef>)> for ScalarValue {
+ fn from((fields, arrays): (Fields, Vec<ArrayRef>)) -> Self {
+ Self::from((fields, arrays, None))
+ }
+}
- Self::Struct(Some(scalars), fields.finish().fields)
+// Wrapper for ScalarValue::Struct that checks the length of the arrays
+impl From<(Fields, Vec<ArrayRef>, Option<NullBuffer>)> for ScalarValue {
Review Comment:
We dont need this after changing Struct with `Scalar<T>`
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Change ScalarValue::Struct to ArrayRef [arrow-datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on PR #7893:
URL: https://github.com/apache/arrow-datafusion/pull/7893#issuecomment-1924916108
> @jayzhan211 -- is this PR ready for a review?
Yes, it keeps getting conflicts, but I think you can take a first scan, unless the conflicts are critical
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Change ScalarValue::Struct to ArrayRef [arrow-datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on PR #7893:
URL: https://github.com/apache/arrow-datafusion/pull/7893#issuecomment-1884909777
@alamb Ready for review!
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Change ScalarValue::Struct to ArrayRef [arrow-datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on code in PR #7893:
URL: https://github.com/apache/arrow-datafusion/pull/7893#discussion_r1447446341
##########
datafusion/common/src/scalar.rs:
##########
@@ -2695,14 +2730,22 @@ impl From<String> for ScalarValue {
}
}
-impl From<Vec<(&str, ScalarValue)>> for ScalarValue {
- fn from(value: Vec<(&str, ScalarValue)>) -> Self {
- let (fields, scalars): (SchemaBuilder, Vec<_>) = value
- .into_iter()
- .map(|(name, scalar)| (Field::new(name, scalar.data_type(), false), scalar))
- .unzip();
+// Wrapper for ScalarValue::Struct that checks the length of the arrays, without nulls
Review Comment:
TODO: Remove these two wrappers, no longer needed after changing to `Scalar<T>`
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Change ScalarValue::Struct to ArrayRef [arrow-datafusion]
Posted by "Weijun-H (via GitHub)" <gi...@apache.org>.
Weijun-H commented on code in PR #7893:
URL: https://github.com/apache/arrow-datafusion/pull/7893#discussion_r1447474057
##########
datafusion/core/src/physical_planner.rs:
##########
@@ -2382,12 +2381,13 @@ mod tests {
Ok(())
}
- /// Return a `null` literal representing a struct type like: `{ a: bool }`
+ // / Return a `null` literal representing a struct type like: `{ a: bool }`
Review Comment:
nit:
```suggestion
/// Return a `null` literal representing a struct type like: `{ a: bool }`
```
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Change ScalarValue::Struct to ArrayRef [arrow-datafusion]
Posted by "mustafasrepo (via GitHub)" <gi...@apache.org>.
mustafasrepo commented on code in PR #7893:
URL: https://github.com/apache/arrow-datafusion/pull/7893#discussion_r1449886112
##########
datafusion/physical-expr/src/aggregate/array_agg_ordered.rs:
##########
@@ -308,18 +319,30 @@ impl OrderSensitiveArrayAggAccumulator {
let fields = ordering_fields(&self.ordering_req, &self.datatypes[1..]);
let struct_field = Fields::from(fields.clone());
- let orderings: Vec<ScalarValue> = self
- .ordering_values
- .iter()
- .map(|ordering| {
- ScalarValue::Struct(Some(ordering.clone()), struct_field.clone())
- })
- .collect();
- let struct_type = DataType::Struct(Fields::from(fields));
+ let mut column_wise_ordering_values = vec![];
+ let num_columns = fields.len();
+ for i in 0..num_columns {
Review Comment:
> I think there might be a better design for StructArray (previous design is based on old ScalarValue::Struct). I avoid changing the logic or data structure in this PR.
>
> May benefit #8558?
I don't think it will benefit #8558 (won't harm either). However it will be a better change anyway.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Change ScalarValue::Struct to ArrayRef [arrow-datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on code in PR #7893:
URL: https://github.com/apache/arrow-datafusion/pull/7893#discussion_r1479148854
##########
datafusion/common/src/scalar.rs:
##########
@@ -2695,14 +2730,22 @@ impl From<String> for ScalarValue {
}
}
-impl From<Vec<(&str, ScalarValue)>> for ScalarValue {
- fn from(value: Vec<(&str, ScalarValue)>) -> Self {
- let (fields, scalars): (SchemaBuilder, Vec<_>) = value
- .into_iter()
- .map(|(name, scalar)| (Field::new(name, scalar.data_type(), false), scalar))
- .unzip();
+// Wrapper for ScalarValue::Struct that checks the length of the arrays, without nulls
Review Comment:
Yes, we haven't changed to Scalar<T> yet.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Change ScalarValue::Struct to ArrayRef [arrow-datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #7893:
URL: https://github.com/apache/arrow-datafusion/pull/7893#discussion_r1478462579
##########
datafusion/core/tests/user_defined/user_defined_aggregates.rs:
##########
@@ -589,29 +589,25 @@ impl FirstSelector {
}
/// Convert to a set of ScalarValues
- fn to_state(&self) -> Vec<ScalarValue> {
- vec![
- ScalarValue::Float64(Some(self.value)),
- ScalarValue::TimestampNanosecond(Some(self.time), None),
- ]
- }
+ fn to_state(&self) -> Result<ScalarValue> {
+ let f64arr = Arc::new(Float64Array::from(vec![self.value])) as ArrayRef;
+ let timearr =
+ Arc::new(TimestampNanosecondArray::from(vec![self.time])) as ArrayRef;
- /// return this selector as a single scalar (struct) value
- fn to_scalar(&self) -> ScalarValue {
- ScalarValue::Struct(Some(self.to_state()), Self::fields())
+ let struct_arr =
+ StructArray::try_new(Self::fields(), vec![f64arr, timearr], None)?;
+ Ok(ScalarValue::Struct(Arc::new(struct_arr)))
Review Comment:
Given this is now returning a struct as the intermediate datatype, I think we also need to update `state_datatypes` to return a `DataType::Struct` to be consistent
##########
datafusion/common/src/scalar.rs:
##########
@@ -2297,14 +2326,20 @@ impl ScalarValue {
Self::Dictionary(key_type.clone(), Box::new(value))
}
DataType::Struct(fields) => {
- let array = as_struct_array(array)?;
+ let struct_arr = as_struct_array(array)?;
Review Comment:
I think this could be implemented using https://docs.rs/arrow/latest/arrow/array/trait.Array.html#tymethod.slice
So like
```rust
let one_element_struct_array = array.slice(index, 1)
.as_struct_array()?
```
Which would be both less code as well as more efficient (no data copies needed)
##########
datafusion/proto/src/logical_plan/to_proto.rs:
##########
@@ -1709,7 +1681,9 @@ fn create_proto_scalar<I, T: FnOnce(&I) -> protobuf::scalar_value::Value>(
Ok(protobuf::ScalarValue { value: Some(value) })
}
-fn encode_scalar_list_value(
+// ScalarValue::List / FixedSizeList / LargeList / Struct are serialized using
+// Arrow IPC messages as a single column RecordBatch
Review Comment:
❤️
##########
datafusion/common/src/scalar.rs:
##########
@@ -2744,6 +2766,25 @@ impl From<Option<&str>> for ScalarValue {
}
}
+impl From<Vec<(&str, ScalarValue)>> for ScalarValue {
Review Comment:
Could we please add a comment explaining what this does?
##########
datafusion/common/src/scalar.rs:
##########
@@ -1377,6 +1402,70 @@ impl ScalarValue {
}};
}
+ fn build_struct_array(
+ scalars: impl IntoIterator<Item = ScalarValue>,
+ ) -> Result<ArrayRef> {
+ let arrays = scalars
+ .into_iter()
+ .map(|s| s.to_array())
+ .collect::<Result<Vec<_>>>()?;
+
+ let first_struct = arrays[0].as_struct_opt();
+ if first_struct.is_none() {
+ return _internal_err!(
+ "Inconsistent types in ScalarValue::iter_to_array. \
+ Expected ScalarValue::Struct, got {:?}",
+ arrays[0].clone()
+ );
+ }
+
+ let mut valid = BooleanBufferBuilder::new(arrays.len());
Review Comment:
What do you think about using https://docs.rs/arrow/latest/arrow/compute/kernels/concat/index.html here? I think you should be able to simply concat the arrays together without having to have special handling (and if concat doesn't support `StructArray` we can potentially file a ticket upstream in arrow-rs)
##########
datafusion/proto/tests/cases/roundtrip_logical_plan.rs:
##########
@@ -693,32 +694,6 @@ impl LogicalExtensionCodec for TopKExtensionCodec {
}
}
-#[test]
-fn scalar_values_error_serialization() {
- let should_fail_on_seralize: Vec<ScalarValue> = vec![
- // Should fail due to empty values
- ScalarValue::Struct(
- Some(vec![]),
Review Comment:
I agree that there is no need to test serializing an empty array as it isn't a valid input anyways
##########
datafusion/common/src/scalar.rs:
##########
@@ -3152,6 +3226,28 @@ impl fmt::Debug for ScalarValue {
ScalarValue::FixedSizeList(_) => write!(f, "FixedSizeList({self})"),
ScalarValue::List(_) => write!(f, "List({self})"),
ScalarValue::LargeList(_) => write!(f, "LargeList({self})"),
+ ScalarValue::Struct(struct_arr) => {
Review Comment:
Likewise here -- perhaps we can use https://docs.rs/arrow/latest/arrow/util/display/fn.array_value_to_string.html again
##########
datafusion/common/src/scalar.rs:
##########
@@ -3078,18 +3132,38 @@ impl fmt::Display for ScalarValue {
ScalarValue::DurationMillisecond(e) => format_option!(f, e)?,
ScalarValue::DurationMicrosecond(e) => format_option!(f, e)?,
ScalarValue::DurationNanosecond(e) => format_option!(f, e)?,
- ScalarValue::Struct(e, fields) => match e {
- Some(l) => write!(
+ ScalarValue::Struct(struct_arr) => {
+ // ScalarValue Struct should always have a single element
+ assert_eq!(struct_arr.len(), 1);
+
+ let columns = struct_arr.columns();
+ let fields = struct_arr.fields();
+ let nulls = struct_arr.nulls();
+
+ write!(
Review Comment:
I think you can use the arrow display code here instead: https://docs.rs/arrow/latest/arrow/util/display/fn.array_value_to_string.html
Using ArrayFormatter directly would save a copy: https://docs.rs/arrow-cast/50.0.0/src/arrow_cast/display.rs.html#930
##########
datafusion/common/src/scalar.rs:
##########
@@ -2695,14 +2730,22 @@ impl From<String> for ScalarValue {
}
}
-impl From<Vec<(&str, ScalarValue)>> for ScalarValue {
- fn from(value: Vec<(&str, ScalarValue)>) -> Self {
- let (fields, scalars): (SchemaBuilder, Vec<_>) = value
- .into_iter()
- .map(|(name, scalar)| (Field::new(name, scalar.data_type(), false), scalar))
- .unzip();
+// Wrapper for ScalarValue::Struct that checks the length of the arrays, without nulls
Review Comment:
Are these still TODO?
##########
datafusion/physical-expr/src/aggregate/array_agg_ordered.rs:
##########
@@ -323,20 +335,32 @@ impl Accumulator for OrderSensitiveArrayAggAccumulator {
impl OrderSensitiveArrayAggAccumulator {
fn evaluate_orderings(&self) -> Result<ScalarValue> {
Review Comment:
Maybe we can file a follow on ticket to track this idea
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Change ScalarValue::Struct to ArrayRef [arrow-datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on PR #7893:
URL: https://github.com/apache/arrow-datafusion/pull/7893#issuecomment-1927041848
Rebase
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Change ScalarValue::Struct to ArrayRef [arrow-datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on code in PR #7893:
URL: https://github.com/apache/arrow-datafusion/pull/7893#discussion_r1447446341
##########
datafusion/common/src/scalar.rs:
##########
@@ -2695,14 +2730,22 @@ impl From<String> for ScalarValue {
}
}
-impl From<Vec<(&str, ScalarValue)>> for ScalarValue {
- fn from(value: Vec<(&str, ScalarValue)>) -> Self {
- let (fields, scalars): (SchemaBuilder, Vec<_>) = value
- .into_iter()
- .map(|(name, scalar)| (Field::new(name, scalar.data_type(), false), scalar))
- .unzip();
+// Wrapper for ScalarValue::Struct that checks the length of the arrays, without nulls
Review Comment:
TODO: Remove these two wrappers, not used.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Change ScalarValue::Struct to ArrayRef [arrow-datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on PR #7893:
URL: https://github.com/apache/arrow-datafusion/pull/7893#issuecomment-1914542913
Rebase
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Change ScalarValue::Struct to ArrayRef [arrow-datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #7893:
URL: https://github.com/apache/arrow-datafusion/pull/7893#issuecomment-1929397781
Thanks @jayzhan211 -- this is looking great. There are a few more outstanding suggestions, but I think we could do them as follow on PRs -- shall I merge this one?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Change ScalarValue::Struct to ArrayRef [arrow-datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on code in PR #7893:
URL: https://github.com/apache/arrow-datafusion/pull/7893#discussion_r1447440740
##########
datafusion/common/src/scalar.rs:
##########
@@ -5292,8 +5557,7 @@ mod tests {
"| col |",
"+---------------------------+",
"| |",
- "| {a: , b: } |",
- "| {a: , b: {ba: , bb: }} |",
+ "| {a: 1, b: } |",
Review Comment:
I think there is no way to construct StructArray like the left-hand side.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Change ScalarValue::Struct to ArrayRef [arrow-datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #7893:
URL: https://github.com/apache/arrow-datafusion/pull/7893
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Change ScalarValue::Struct to ArrayRef [arrow-datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on code in PR #7893:
URL: https://github.com/apache/arrow-datafusion/pull/7893#discussion_r1447454548
##########
datafusion/physical-expr/src/aggregate/array_agg_ordered.rs:
##########
@@ -308,18 +319,30 @@ impl OrderSensitiveArrayAggAccumulator {
let fields = ordering_fields(&self.ordering_req, &self.datatypes[1..]);
let struct_field = Fields::from(fields.clone());
- let orderings: Vec<ScalarValue> = self
- .ordering_values
- .iter()
- .map(|ordering| {
- ScalarValue::Struct(Some(ordering.clone()), struct_field.clone())
- })
- .collect();
- let struct_type = DataType::Struct(Fields::from(fields));
+ let mut column_wise_ordering_values = vec![];
+ let num_columns = fields.len();
+ for i in 0..num_columns {
Review Comment:
I think there might be a better design for StructArray (previous design is based on old ScalarValue::Struct). I avoid to change the logic or data structure in this PR.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Change ScalarValue::Struct to ArrayRef [arrow-datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on code in PR #7893:
URL: https://github.com/apache/arrow-datafusion/pull/7893#discussion_r1447454548
##########
datafusion/physical-expr/src/aggregate/array_agg_ordered.rs:
##########
@@ -308,18 +319,30 @@ impl OrderSensitiveArrayAggAccumulator {
let fields = ordering_fields(&self.ordering_req, &self.datatypes[1..]);
let struct_field = Fields::from(fields.clone());
- let orderings: Vec<ScalarValue> = self
- .ordering_values
- .iter()
- .map(|ordering| {
- ScalarValue::Struct(Some(ordering.clone()), struct_field.clone())
- })
- .collect();
- let struct_type = DataType::Struct(Fields::from(fields));
+ let mut column_wise_ordering_values = vec![];
+ let num_columns = fields.len();
+ for i in 0..num_columns {
Review Comment:
I think there might be a better design for StructArray (previous design is based on old ScalarValue::Struct). I avoid any other change in this PR.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Change ScalarValue::Struct to ArrayRef [arrow-datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #7893:
URL: https://github.com/apache/arrow-datafusion/pull/7893#discussion_r1481436570
##########
datafusion/common/src/scalar.rs:
##########
@@ -1377,6 +1402,70 @@ impl ScalarValue {
}};
}
+ fn build_struct_array(
+ scalars: impl IntoIterator<Item = ScalarValue>,
+ ) -> Result<ArrayRef> {
+ let arrays = scalars
+ .into_iter()
+ .map(|s| s.to_array())
+ .collect::<Result<Vec<_>>>()?;
+
+ let first_struct = arrays[0].as_struct_opt();
+ if first_struct.is_none() {
+ return _internal_err!(
+ "Inconsistent types in ScalarValue::iter_to_array. \
+ Expected ScalarValue::Struct, got {:?}",
+ arrays[0].clone()
+ );
+ }
+
+ let mut valid = BooleanBufferBuilder::new(arrays.len());
Review Comment:
https://github.com/apache/arrow-datafusion/issues/9145
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Change ScalarValue::Struct to ArrayRef [arrow-datafusion]
Posted by "Weijun-H (via GitHub)" <gi...@apache.org>.
Weijun-H commented on code in PR #7893:
URL: https://github.com/apache/arrow-datafusion/pull/7893#discussion_r1458573584
##########
datafusion/common/src/scalar.rs:
##########
@@ -370,6 +370,34 @@ impl PartialOrd for ScalarValue {
partial_cmp_list(arr1.as_ref(), arr2.as_ref())
}
(List(_), _) | (LargeList(_), _) | (FixedSizeList(_), _) => None,
+ (Struct(struct_arr1), Struct(struct_arr2)) => {
+ if struct_arr1.len() != struct_arr2.len() {
+ return None;
+ }
+
+ if struct_arr1.data_type() != struct_arr2.data_type() {
+ return None;
+ }
+
+ for col_index in 0..struct_arr1.num_columns() {
+ let arr1 = struct_arr1.column(col_index);
+ let arr2 = struct_arr2.column(col_index);
+
+ let lt_res = arrow::compute::kernels::cmp::lt(arr1, arr2).ok()?;
+ let eq_res = arrow::compute::kernels::cmp::eq(arr1, arr2).ok()?;
+
+ for j in 0..lt_res.len() {
+ if lt_res.is_valid(j) && lt_res.value(j) {
+ return Some(Ordering::Less);
+ }
+ if eq_res.is_valid(j) && !eq_res.value(j) {
+ return Some(Ordering::Greater);
+ }
+ }
+ }
+ Some(Ordering::Equal)
Review Comment:
What do you think put it into function `partial_cmp_struct` to make the code clear?
##########
datafusion/common/src/scalar.rs:
##########
@@ -1376,6 +1395,70 @@ impl ScalarValue {
}};
}
+ fn build_struct_array(
Review Comment:
👍
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Change ScalarValue::Struct to ArrayRef [arrow-datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on code in PR #7893:
URL: https://github.com/apache/arrow-datafusion/pull/7893#discussion_r1443647677
##########
datafusion/proto/tests/cases/roundtrip_logical_plan.rs:
##########
@@ -693,32 +694,6 @@ impl LogicalExtensionCodec for TopKExtensionCodec {
}
}
-#[test]
-fn scalar_values_error_serialization() {
- let should_fail_on_seralize: Vec<ScalarValue> = vec![
- // Should fail due to empty values
- ScalarValue::Struct(
- Some(vec![]),
Review Comment:
Move to `round_trip_scalar_values`, since it is able to serialized
```
ScalarValue::try_from(&DataType::Struct(Fields::from(vec![
Field::new("a", DataType::Int32, true),
Field::new("a", DataType::Boolean, false),
])))
.unwrap(),
```
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Change ScalarValue::Struct to ArrayRef [arrow-datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on code in PR #7893:
URL: https://github.com/apache/arrow-datafusion/pull/7893#discussion_r1447454548
##########
datafusion/physical-expr/src/aggregate/array_agg_ordered.rs:
##########
@@ -308,18 +319,30 @@ impl OrderSensitiveArrayAggAccumulator {
let fields = ordering_fields(&self.ordering_req, &self.datatypes[1..]);
let struct_field = Fields::from(fields.clone());
- let orderings: Vec<ScalarValue> = self
- .ordering_values
- .iter()
- .map(|ordering| {
- ScalarValue::Struct(Some(ordering.clone()), struct_field.clone())
- })
- .collect();
- let struct_type = DataType::Struct(Fields::from(fields));
+ let mut column_wise_ordering_values = vec![];
+ let num_columns = fields.len();
+ for i in 0..num_columns {
Review Comment:
I think there might be a better design for StructArray (previous design is based on old ScalarValue::Struct). I did not change too much in this PR.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Change ScalarValue::Struct to ArrayRef [arrow-datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on code in PR #7893:
URL: https://github.com/apache/arrow-datafusion/pull/7893#discussion_r1447448644
##########
datafusion/common/src/scalar.rs:
##########
@@ -2988,18 +3034,38 @@ impl fmt::Display for ScalarValue {
ScalarValue::DurationMillisecond(e) => format_option!(f, e)?,
ScalarValue::DurationMicrosecond(e) => format_option!(f, e)?,
ScalarValue::DurationNanosecond(e) => format_option!(f, e)?,
- ScalarValue::Struct(e, fields) => match e {
- Some(l) => write!(
+ ScalarValue::Struct(struct_arr) => {
+ // ScalarValue Struct should always have a single element
+ assert_eq!(struct_arr.len(), 1);
+
+ let columns = struct_arr.columns();
+ let fields = struct_arr.fields();
+ let nulls = struct_arr.nulls();
+
+ write!(
Review Comment:
to align with the old format
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Change ScalarValue::Struct to ArrayRef [arrow-datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on code in PR #7893:
URL: https://github.com/apache/arrow-datafusion/pull/7893#discussion_r1447444632
##########
datafusion/sqllogictest/test_files/explain.slt:
##########
@@ -380,3 +380,15 @@ Projection: List([[1, 2, 3], [4, 5, 6]]) AS make_array(make_array(Int64(1),Int64
physical_plan
ProjectionExec: expr=[[[1, 2, 3], [4, 5, 6]] as make_array(make_array(Int64(1),Int64(2),Int64(3)),make_array(Int64(4),Int64(5),Int64(6)))]
--PlaceholderRowExec
+
+# Explain Struct
+
+query TT
+explain select struct(1, 2.3, 'abc');
+----
+logical_plan
+Projection: Struct({c0:Int64(1),c1:Float64(2.3),c2:Utf8("abc")}) AS struct(Int64(1),Float64(2.3),Utf8("abc"))
Review Comment:
test format
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Change ScalarValue::Struct to ArrayRef [arrow-datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on code in PR #7893:
URL: https://github.com/apache/arrow-datafusion/pull/7893#discussion_r1459025527
##########
datafusion/physical-expr/src/aggregate/array_agg_ordered.rs:
##########
@@ -323,20 +335,32 @@ impl Accumulator for OrderSensitiveArrayAggAccumulator {
impl OrderSensitiveArrayAggAccumulator {
fn evaluate_orderings(&self) -> Result<ScalarValue> {
Review Comment:
@mustafasrepo Have you considered changing `values: Vec<ScalarValue>` and `ordering_values: Vec<Vec<ScalarValue>>,` from vec to vecdeque for OrderSensitiveArrayAggAccumulator too? If we can have VecDequeue here, we can avoid `into()` and reuse evalute_orderings() for both Ordersensitive And NthValue. They do the similar things instead one is Vec<T>, another is VecDeque<T>.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Change ScalarValue::Struct to ArrayRef [arrow-datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #7893:
URL: https://github.com/apache/arrow-datafusion/pull/7893#issuecomment-1891038953
Thanks @jayzhan211 -- I will try and get this reviewed sometime over this next week
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Change ScalarValue::Struct to ArrayRef [arrow-datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on code in PR #7893:
URL: https://github.com/apache/arrow-datafusion/pull/7893#discussion_r1447454548
##########
datafusion/physical-expr/src/aggregate/array_agg_ordered.rs:
##########
@@ -308,18 +319,30 @@ impl OrderSensitiveArrayAggAccumulator {
let fields = ordering_fields(&self.ordering_req, &self.datatypes[1..]);
let struct_field = Fields::from(fields.clone());
- let orderings: Vec<ScalarValue> = self
- .ordering_values
- .iter()
- .map(|ordering| {
- ScalarValue::Struct(Some(ordering.clone()), struct_field.clone())
- })
- .collect();
- let struct_type = DataType::Struct(Fields::from(fields));
+ let mut column_wise_ordering_values = vec![];
+ let num_columns = fields.len();
+ for i in 0..num_columns {
Review Comment:
I think there might be a better design for StructArray (previous design is based on old ScalarValue::Struct). I avoid changing the logic or data structure in this PR.
May benefit #8558?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Change ScalarValue::Struct to ArrayRef [arrow-datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on code in PR #7893:
URL: https://github.com/apache/arrow-datafusion/pull/7893#discussion_r1447448328
##########
datafusion/common/src/scalar.rs:
##########
@@ -2988,18 +3034,38 @@ impl fmt::Display for ScalarValue {
ScalarValue::DurationMillisecond(e) => format_option!(f, e)?,
ScalarValue::DurationMicrosecond(e) => format_option!(f, e)?,
ScalarValue::DurationNanosecond(e) => format_option!(f, e)?,
- ScalarValue::Struct(e, fields) => match e {
- Some(l) => write!(
+ ScalarValue::Struct(struct_arr) => {
+ // ScalarValue Struct should always have a single element
+ assert_eq!(struct_arr.len(), 1);
+
+ let columns = struct_arr.columns();
+ let fields = struct_arr.fields();
+ let nulls = struct_arr.nulls();
+
+ write!(
f,
"{{{}}}",
- l.iter()
+ columns
+ .iter()
.zip(fields.iter())
- .map(|(value, field)| format!("{}:{}", field.name(), value))
+ .enumerate()
+ .map(|(index, (column, field))| {
+ if nulls.is_some_and(|b| b.is_null(index)) {
+ format!("{}:NULL", field.name())
Review Comment:
to align with the old format
##########
datafusion/common/src/scalar.rs:
##########
@@ -2988,18 +3034,38 @@ impl fmt::Display for ScalarValue {
ScalarValue::DurationMillisecond(e) => format_option!(f, e)?,
ScalarValue::DurationMicrosecond(e) => format_option!(f, e)?,
ScalarValue::DurationNanosecond(e) => format_option!(f, e)?,
- ScalarValue::Struct(e, fields) => match e {
- Some(l) => write!(
+ ScalarValue::Struct(struct_arr) => {
+ // ScalarValue Struct should always have a single element
+ assert_eq!(struct_arr.len(), 1);
+
+ let columns = struct_arr.columns();
+ let fields = struct_arr.fields();
+ let nulls = struct_arr.nulls();
+
+ write!(
f,
"{{{}}}",
- l.iter()
+ columns
+ .iter()
.zip(fields.iter())
- .map(|(value, field)| format!("{}:{}", field.name(), value))
+ .enumerate()
+ .map(|(index, (column, field))| {
+ if nulls.is_some_and(|b| b.is_null(index)) {
+ format!("{}:NULL", field.name())
Review Comment:
to align with the old format
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Change ScalarValue::Struct to ArrayRef [arrow-datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on code in PR #7893:
URL: https://github.com/apache/arrow-datafusion/pull/7893#discussion_r1447448644
##########
datafusion/common/src/scalar.rs:
##########
@@ -2988,18 +3034,38 @@ impl fmt::Display for ScalarValue {
ScalarValue::DurationMillisecond(e) => format_option!(f, e)?,
ScalarValue::DurationMicrosecond(e) => format_option!(f, e)?,
ScalarValue::DurationNanosecond(e) => format_option!(f, e)?,
- ScalarValue::Struct(e, fields) => match e {
- Some(l) => write!(
+ ScalarValue::Struct(struct_arr) => {
+ // ScalarValue Struct should always have a single element
+ assert_eq!(struct_arr.len(), 1);
+
+ let columns = struct_arr.columns();
+ let fields = struct_arr.fields();
+ let nulls = struct_arr.nulls();
+
+ write!(
Review Comment:
to align with the old format
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Change ScalarValue::Struct to ArrayRef [arrow-datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on code in PR #7893:
URL: https://github.com/apache/arrow-datafusion/pull/7893#discussion_r1448143623
##########
datafusion/physical-expr/src/aggregate/array_agg_ordered.rs:
##########
@@ -224,28 +229,32 @@ impl Accumulator for OrderSensitiveArrayAggAccumulator {
partition_values.push(self.values.clone());
partition_ordering_values.push(self.ordering_values.clone());
+ assert!(as_list_array(array_agg_values).is_ok());
+ // Convert array to Scalars to sort them easily. Convert back to array at evaluation.
let array_agg_res =
ScalarValue::convert_array_to_scalar_vec(array_agg_values)?;
-
- for v in array_agg_res.into_iter() {
- partition_values.push(v);
- }
+ partition_values.extend(array_agg_res);
let orderings = ScalarValue::convert_array_to_scalar_vec(agg_orderings)?;
-
for partition_ordering_rows in orderings.into_iter() {
// Extract value from struct to ordering_rows for each group/partition
let ordering_value = partition_ordering_rows.into_iter().map(|ordering_row| {
- if let ScalarValue::Struct(Some(ordering_columns_per_row), _) = ordering_row {
+ if let ScalarValue::Struct(s) = ordering_row {
+ let mut ordering_columns_per_row = vec![];
+
+ for column in s.columns() {
+ let sv = ScalarValue::try_from_array(column, 0)?;
+ ordering_columns_per_row.push(sv);
+ }
+
Ok(ordering_columns_per_row)
} else {
exec_err!(
Review Comment:
todo: internal_err
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Change ScalarValue::Struct to ArrayRef [arrow-datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on code in PR #7893:
URL: https://github.com/apache/arrow-datafusion/pull/7893#discussion_r1443643473
##########
datafusion/proto/tests/cases/roundtrip_logical_plan.rs:
##########
@@ -693,32 +694,6 @@ impl LogicalExtensionCodec for TopKExtensionCodec {
}
}
-#[test]
-fn scalar_values_error_serialization() {
- let should_fail_on_seralize: Vec<ScalarValue> = vec![
- // Should fail due to empty values
- ScalarValue::Struct(
- Some(vec![]),
Review Comment:
Not supposed to create empty array, so just removed the test
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Change ScalarValue::Struct to ArrayRef [arrow-datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on PR #7893:
URL: https://github.com/apache/arrow-datafusion/pull/7893#issuecomment-1929665085
> Thanks @jayzhan211 -- this is looking great. There are a few more outstanding suggestions, but I think we could do them as follow on PRs -- shall I merge this one?
Sure!
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Change ScalarValue::Struct to ArrayRef [arrow-datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #7893:
URL: https://github.com/apache/arrow-datafusion/pull/7893#issuecomment-1931997736
Thanks again @jayzhan211
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Change ScalarValue::Struct to ArrayRef [arrow-datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #7893:
URL: https://github.com/apache/arrow-datafusion/pull/7893#issuecomment-1927284298
Sorry -- starting to look now
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org