You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "rspears74 (via GitHub)" <gi...@apache.org> on 2023/12/16 00:13:57 UTC
[PR] ScalarValue::List type narrowing [arrow-datafusion]
rspears74 opened a new pull request, #8562:
URL: https://github.com/apache/arrow-datafusion/pull/8562
## Which issue does this PR close?
Closes #8472
## Rationale for this change
It's much preferable to constrain `ScalarValue::List` to accept only an `Arc<ListArray>` rather than any `ArrayRef` and fail randomly. Similar for `ScalarValue::LargeList` and `ScalarValue::FixedSizeList`.
## What changes are included in this PR?
Changed the type signature of `ScalarValue::List`, `ScalarValue::LargeList`, and `ScalarValue::FixedSizedList` and any cascading changes that follow from this. Deleted one test that is no longer valid (and was arguably not valid before considering the implied type constraints).
## Are these changes tested?
Tests have been updated where needed.
## Are there any user-facing changes?
Users are now only allowed to pass a `ListArray` to `ScalarValue::List`, a `LargeListArray` to `ScalarValue::LargeList`, and a `FixedSizeListArray` to `ScalarValue::FixedSizeList`.
--
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::{List, LargeList, FixedSizedList}` to take specific types rather than `ArrayRef` [arrow-datafusion]
Posted by "rspears74 (via GitHub)" <gi...@apache.org>.
rspears74 commented on code in PR #8562:
URL: https://github.com/apache/arrow-datafusion/pull/8562#discussion_r1443859383
##########
datafusion/common/src/scalar.rs:
##########
@@ -2433,11 +2455,14 @@ impl ScalarValue {
ScalarValue::LargeBinary(val) => {
eq_array_primitive!(array, index, LargeBinaryArray, val)?
}
- ScalarValue::List(arr)
- | ScalarValue::LargeList(arr)
- | ScalarValue::FixedSizeList(arr) => {
- let right = array.slice(index, 1);
- arr == &right
+ ScalarValue::List(arr) => {
+ Self::eq_array_list(&(arr.to_owned() as ArrayRef), array, index)
+ }
+ ScalarValue::LargeList(arr) => {
+ Self::eq_array_list(&(arr.to_owned() as ArrayRef), array, index)
+ }
+ ScalarValue::FixedSizeList(arr) => {
+ Self::eq_array_list(&(arr.to_owned() as ArrayRef), array, index)
Review Comment:
Yeah, I tried this, but the args to `List`, `LargeList`, and `FixedSizeList` are all different, so they can't be put in the same match arm.
--
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::{List, LargeList, FixedSizedList}` to take specific types rather than `ArrayRef` [arrow-datafusion]
Posted by "rspears74 (via GitHub)" <gi...@apache.org>.
rspears74 commented on code in PR #8562:
URL: https://github.com/apache/arrow-datafusion/pull/8562#discussion_r1430356860
##########
datafusion/proto/src/logical_plan/to_proto.rs:
##########
@@ -1159,13 +1160,105 @@ impl TryFrom<&ScalarValue> for protobuf::ScalarValue {
}
// ScalarValue::List and ScalarValue::FixedSizeList are serialized using
// Arrow IPC messages as a single column RecordBatch
- ScalarValue::List(arr)
- | ScalarValue::LargeList(arr)
- | ScalarValue::FixedSizeList(arr) => {
+ ScalarValue::List(arr) => {
// Wrap in a "field_name" column
let batch = RecordBatch::try_from_iter(vec![(
"field_name",
- arr.to_owned(),
+ arr.to_owned() as ArrayRef,
+ )])
+ .map_err(|e| {
+ Error::General( format!("Error creating temporary batch while encoding ScalarValue::List: {e}"))
+ })?;
+
+ let gen = IpcDataGenerator {};
+ let mut dict_tracker = DictionaryTracker::new(false);
+ let (_, encoded_message) = gen
+ .encoded_batch(&batch, &mut dict_tracker, &Default::default())
+ .map_err(|e| {
+ Error::General(format!(
+ "Error encoding ScalarValue::List as IPC: {e}"
+ ))
+ })?;
+
+ let schema: protobuf::Schema = batch.schema().try_into()?;
+
+ let scalar_list_value = protobuf::ScalarListValue {
+ ipc_message: encoded_message.ipc_message,
+ arrow_data: encoded_message.arrow_data,
+ schema: Some(schema),
+ };
+
+ match val {
+ ScalarValue::List(_) => Ok(protobuf::ScalarValue {
Review Comment:
Ah good catch on these!
--
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::{List, LargeList, FixedSizedList}` to take specific types rather than `ArrayRef` [arrow-datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on code in PR #8562:
URL: https://github.com/apache/arrow-datafusion/pull/8562#discussion_r1430778346
##########
datafusion/common/src/scalar.rs:
##########
@@ -3300,33 +3409,6 @@ mod tests {
assert_eq!(result, &expected);
}
- #[test]
Review Comment:
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::{List, LargeList, FixedSizedList}` to take specific types rather than `ArrayRef` [arrow-datafusion]
Posted by "rspears74 (via GitHub)" <gi...@apache.org>.
rspears74 commented on code in PR #8562:
URL: https://github.com/apache/arrow-datafusion/pull/8562#discussion_r1443860094
##########
datafusion/common/src/scalar.rs:
##########
@@ -142,13 +143,13 @@ pub enum ScalarValue {
/// Fixed size list scalar.
///
/// The array must be a FixedSizeListArray with length 1.
- FixedSizeList(ArrayRef),
+ FixedSizeList(Arc<FixedSizeListArray>),
Review Comment:
Personally I like the enforcement of length 1 (I mentioned this problem initially in the issue I opened), but it does seem to be a bit cumbersome to deal with.
--
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::{List, LargeList, FixedSizedList}` to take specific types rather than `ArrayRef` [arrow-datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on PR #8562:
URL: https://github.com/apache/arrow-datafusion/pull/8562#issuecomment-1876156862
@alamb It seems you forget to trace this PR in https://github.com/apache/arrow-datafusion/issues/8704?
--
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::{List, LargeList, FixedSizedList}` to take specific types rather than `ArrayRef` [arrow-datafusion]
Posted by "rspears74 (via GitHub)" <gi...@apache.org>.
rspears74 commented on code in PR #8562:
URL: https://github.com/apache/arrow-datafusion/pull/8562#discussion_r1443521603
##########
datafusion/proto/src/logical_plan/to_proto.rs:
##########
@@ -1189,24 +1188,79 @@ impl TryFrom<&ScalarValue> for protobuf::ScalarValue {
schema: Some(schema),
};
- match val {
- ScalarValue::List(_) => Ok(protobuf::ScalarValue {
- value: Some(protobuf::scalar_value::Value::ListValue(
- scalar_list_value,
- )),
- }),
- ScalarValue::LargeList(_) => Ok(protobuf::ScalarValue {
- value: Some(protobuf::scalar_value::Value::LargeListValue(
- scalar_list_value,
- )),
- }),
- ScalarValue::FixedSizeList(_) => Ok(protobuf::ScalarValue {
- value: Some(protobuf::scalar_value::Value::FixedSizeListValue(
- scalar_list_value,
- )),
- }),
- _ => unreachable!(),
- }
+ Ok(protobuf::ScalarValue {
+ value: Some(protobuf::scalar_value::Value::ListValue(
+ scalar_list_value,
+ )),
+ })
+ }
+ ScalarValue::LargeList(arr) => {
Review Comment:
This one was easy enough to implement.
--
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::{List, LargeList, FixedSizedList}` to take specific types rather than `ArrayRef` [arrow-datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #8562:
URL: https://github.com/apache/arrow-datafusion/pull/8562
--
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::{List, LargeList, FixedSizedList}` to take specific types rather than `ArrayRef` [arrow-datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #8562:
URL: https://github.com/apache/arrow-datafusion/pull/8562#discussion_r1443407346
##########
datafusion/proto/src/logical_plan/to_proto.rs:
##########
@@ -1189,24 +1188,79 @@ impl TryFrom<&ScalarValue> for protobuf::ScalarValue {
schema: Some(schema),
};
- match val {
- ScalarValue::List(_) => Ok(protobuf::ScalarValue {
- value: Some(protobuf::scalar_value::Value::ListValue(
- scalar_list_value,
- )),
- }),
- ScalarValue::LargeList(_) => Ok(protobuf::ScalarValue {
- value: Some(protobuf::scalar_value::Value::LargeListValue(
- scalar_list_value,
- )),
- }),
- ScalarValue::FixedSizeList(_) => Ok(protobuf::ScalarValue {
- value: Some(protobuf::scalar_value::Value::FixedSizeListValue(
- scalar_list_value,
- )),
- }),
- _ => unreachable!(),
- }
+ Ok(protobuf::ScalarValue {
+ value: Some(protobuf::scalar_value::Value::ListValue(
+ scalar_list_value,
+ )),
+ })
+ }
+ ScalarValue::LargeList(arr) => {
Review Comment:
Would it be possible to avoid the copy/paste in this method by making a function? Something like the following?
```rust
fn encode_scalar_list_value(array: &Array) -> Result<protobuf::ScalarListValue> {
...
}
```
##########
datafusion/common/src/scalar.rs:
##########
@@ -359,29 +359,77 @@ impl PartialOrd for ScalarValue {
(FixedSizeBinary(_, _), _) => None,
(LargeBinary(v1), LargeBinary(v2)) => v1.partial_cmp(v2),
(LargeBinary(_), _) => None,
- (List(arr1), List(arr2))
- | (FixedSizeList(arr1), FixedSizeList(arr2))
- | (LargeList(arr1), LargeList(arr2)) => {
- // ScalarValue::List / ScalarValue::FixedSizeList / ScalarValue::LargeList are ensure to have length 1
+ // ScalarValue::List / ScalarValue::FixedSizeList / ScalarValue::LargeList are ensure to have length 1
+ (List(arr1), List(arr2)) => {
assert_eq!(arr1.len(), 1);
assert_eq!(arr2.len(), 1);
if arr1.data_type() != arr2.data_type() {
return None;
}
- fn first_array_for_list(arr: &ArrayRef) -> ArrayRef {
- if let Some(arr) = arr.as_list_opt::<i32>() {
- arr.value(0)
- } else if let Some(arr) = arr.as_list_opt::<i64>() {
- arr.value(0)
- } else if let Some(arr) = arr.as_fixed_size_list_opt() {
- arr.value(0)
- } else {
- unreachable!("Since only List / LargeList / FixedSizeList are supported, this should never happen")
+ fn first_array_for_list(arr: &Arc<ListArray>) -> ArrayRef {
Review Comment:
It would be really nice here to avoid the redundant copy/paste code, perhaps by making a function with signature like this
```
fn cmp_list_array(arr1: &dyn Array, arr2: &dyn Array) -> Option<Ordering> {
..
}
```
Which then you can call with `Arc<ListArray>` and `Arc<LargeListArray>` types
--
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::{List, LargeList, FixedSizedList}` to take specific types rather than `ArrayRef` [arrow-datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #8562:
URL: https://github.com/apache/arrow-datafusion/pull/8562#issuecomment-1880050346
> Not sure why the MSRV check is failing?
You may need to merge up from main -- I think this was fixed on main recently
--
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::{List, LargeList, FixedSizedList}` to take specific types rather than `ArrayRef` [arrow-datafusion]
Posted by "rspears74 (via GitHub)" <gi...@apache.org>.
rspears74 commented on PR #8562:
URL: https://github.com/apache/arrow-datafusion/pull/8562#issuecomment-1879877160
Not sure why the MSRV check is failing?
--
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] ScalarValue::List type narrowing [arrow-datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on code in PR #8562:
URL: https://github.com/apache/arrow-datafusion/pull/8562#discussion_r1428611525
##########
datafusion/common/src/scalar.rs:
##########
@@ -3300,33 +3409,6 @@ mod tests {
assert_eq!(result, &expected);
}
- #[test]
Review Comment:
Is ScalarValue::eq_array test for List elsewhere? Otherwise, we can add the test for it.
##########
datafusion/proto/src/logical_plan/to_proto.rs:
##########
@@ -1159,13 +1160,105 @@ impl TryFrom<&ScalarValue> for protobuf::ScalarValue {
}
// ScalarValue::List and ScalarValue::FixedSizeList are serialized using
// Arrow IPC messages as a single column RecordBatch
- ScalarValue::List(arr)
- | ScalarValue::LargeList(arr)
- | ScalarValue::FixedSizeList(arr) => {
+ ScalarValue::List(arr) => {
// Wrap in a "field_name" column
let batch = RecordBatch::try_from_iter(vec![(
"field_name",
- arr.to_owned(),
+ arr.to_owned() as ArrayRef,
+ )])
+ .map_err(|e| {
+ Error::General( format!("Error creating temporary batch while encoding ScalarValue::List: {e}"))
+ })?;
+
+ let gen = IpcDataGenerator {};
+ let mut dict_tracker = DictionaryTracker::new(false);
+ let (_, encoded_message) = gen
+ .encoded_batch(&batch, &mut dict_tracker, &Default::default())
+ .map_err(|e| {
+ Error::General(format!(
+ "Error encoding ScalarValue::List as IPC: {e}"
+ ))
+ })?;
+
+ let schema: protobuf::Schema = batch.schema().try_into()?;
+
+ let scalar_list_value = protobuf::ScalarListValue {
+ ipc_message: encoded_message.ipc_message,
+ arrow_data: encoded_message.arrow_data,
+ schema: Some(schema),
+ };
+
+ match val {
+ ScalarValue::List(_) => Ok(protobuf::ScalarValue {
+ value: Some(protobuf::scalar_value::Value::ListValue(
+ scalar_list_value,
+ )),
+ }),
+ ScalarValue::LargeList(_) => Ok(protobuf::ScalarValue {
+ value: Some(protobuf::scalar_value::Value::LargeListValue(
+ scalar_list_value,
+ )),
+ }),
+ ScalarValue::FixedSizeList(_) => Ok(protobuf::ScalarValue {
+ value: Some(protobuf::scalar_value::Value::FixedSizeListValue(
+ scalar_list_value,
+ )),
+ }),
+ _ => unreachable!(),
+ }
+ }
+ ScalarValue::LargeList(arr) => {
+ // Wrap in a "field_name" column
+ let batch = RecordBatch::try_from_iter(vec![(
+ "field_name",
+ arr.to_owned() as ArrayRef,
+ )])
+ .map_err(|e| {
+ Error::General( format!("Error creating temporary batch while encoding ScalarValue::List: {e}"))
+ })?;
+
+ let gen = IpcDataGenerator {};
+ let mut dict_tracker = DictionaryTracker::new(false);
+ let (_, encoded_message) = gen
+ .encoded_batch(&batch, &mut dict_tracker, &Default::default())
+ .map_err(|e| {
+ Error::General(format!(
+ "Error encoding ScalarValue::List as IPC: {e}"
+ ))
+ })?;
+
+ let schema: protobuf::Schema = batch.schema().try_into()?;
+
+ let scalar_list_value = protobuf::ScalarListValue {
+ ipc_message: encoded_message.ipc_message,
+ arrow_data: encoded_message.arrow_data,
+ schema: Some(schema),
+ };
+
+ match val {
+ ScalarValue::List(_) => Ok(protobuf::ScalarValue {
+ value: Some(protobuf::scalar_value::Value::ListValue(
+ scalar_list_value,
+ )),
+ }),
+ ScalarValue::LargeList(_) => Ok(protobuf::ScalarValue {
Review Comment:
Same
##########
datafusion/proto/src/logical_plan/to_proto.rs:
##########
@@ -1159,13 +1160,105 @@ impl TryFrom<&ScalarValue> for protobuf::ScalarValue {
}
// ScalarValue::List and ScalarValue::FixedSizeList are serialized using
// Arrow IPC messages as a single column RecordBatch
- ScalarValue::List(arr)
- | ScalarValue::LargeList(arr)
- | ScalarValue::FixedSizeList(arr) => {
+ ScalarValue::List(arr) => {
// Wrap in a "field_name" column
let batch = RecordBatch::try_from_iter(vec![(
"field_name",
- arr.to_owned(),
+ arr.to_owned() as ArrayRef,
+ )])
+ .map_err(|e| {
+ Error::General( format!("Error creating temporary batch while encoding ScalarValue::List: {e}"))
+ })?;
+
+ let gen = IpcDataGenerator {};
+ let mut dict_tracker = DictionaryTracker::new(false);
+ let (_, encoded_message) = gen
+ .encoded_batch(&batch, &mut dict_tracker, &Default::default())
+ .map_err(|e| {
+ Error::General(format!(
+ "Error encoding ScalarValue::List as IPC: {e}"
+ ))
+ })?;
+
+ let schema: protobuf::Schema = batch.schema().try_into()?;
+
+ let scalar_list_value = protobuf::ScalarListValue {
+ ipc_message: encoded_message.ipc_message,
+ arrow_data: encoded_message.arrow_data,
+ schema: Some(schema),
+ };
+
+ match val {
+ ScalarValue::List(_) => Ok(protobuf::ScalarValue {
Review Comment:
I think val is already matched to ScalarValue::List in this block, just process it.
##########
datafusion/proto/src/logical_plan/to_proto.rs:
##########
@@ -1159,13 +1160,105 @@ impl TryFrom<&ScalarValue> for protobuf::ScalarValue {
}
// ScalarValue::List and ScalarValue::FixedSizeList are serialized using
// Arrow IPC messages as a single column RecordBatch
- ScalarValue::List(arr)
- | ScalarValue::LargeList(arr)
- | ScalarValue::FixedSizeList(arr) => {
+ ScalarValue::List(arr) => {
// Wrap in a "field_name" column
let batch = RecordBatch::try_from_iter(vec![(
"field_name",
- arr.to_owned(),
+ arr.to_owned() as ArrayRef,
+ )])
+ .map_err(|e| {
+ Error::General( format!("Error creating temporary batch while encoding ScalarValue::List: {e}"))
+ })?;
+
+ let gen = IpcDataGenerator {};
+ let mut dict_tracker = DictionaryTracker::new(false);
+ let (_, encoded_message) = gen
+ .encoded_batch(&batch, &mut dict_tracker, &Default::default())
+ .map_err(|e| {
+ Error::General(format!(
+ "Error encoding ScalarValue::List as IPC: {e}"
+ ))
+ })?;
+
+ let schema: protobuf::Schema = batch.schema().try_into()?;
+
+ let scalar_list_value = protobuf::ScalarListValue {
+ ipc_message: encoded_message.ipc_message,
+ arrow_data: encoded_message.arrow_data,
+ schema: Some(schema),
+ };
+
+ match val {
+ ScalarValue::List(_) => Ok(protobuf::ScalarValue {
+ value: Some(protobuf::scalar_value::Value::ListValue(
+ scalar_list_value,
+ )),
+ }),
+ ScalarValue::LargeList(_) => Ok(protobuf::ScalarValue {
+ value: Some(protobuf::scalar_value::Value::LargeListValue(
+ scalar_list_value,
+ )),
+ }),
+ ScalarValue::FixedSizeList(_) => Ok(protobuf::ScalarValue {
+ value: Some(protobuf::scalar_value::Value::FixedSizeListValue(
+ scalar_list_value,
+ )),
+ }),
+ _ => unreachable!(),
+ }
+ }
+ ScalarValue::LargeList(arr) => {
+ // Wrap in a "field_name" column
+ let batch = RecordBatch::try_from_iter(vec![(
+ "field_name",
+ arr.to_owned() as ArrayRef,
+ )])
+ .map_err(|e| {
+ Error::General( format!("Error creating temporary batch while encoding ScalarValue::List: {e}"))
+ })?;
+
+ let gen = IpcDataGenerator {};
+ let mut dict_tracker = DictionaryTracker::new(false);
+ let (_, encoded_message) = gen
+ .encoded_batch(&batch, &mut dict_tracker, &Default::default())
+ .map_err(|e| {
+ Error::General(format!(
+ "Error encoding ScalarValue::List as IPC: {e}"
+ ))
+ })?;
+
+ let schema: protobuf::Schema = batch.schema().try_into()?;
+
+ let scalar_list_value = protobuf::ScalarListValue {
+ ipc_message: encoded_message.ipc_message,
+ arrow_data: encoded_message.arrow_data,
+ schema: Some(schema),
+ };
+
+ match val {
+ ScalarValue::List(_) => Ok(protobuf::ScalarValue {
+ value: Some(protobuf::scalar_value::Value::ListValue(
+ scalar_list_value,
+ )),
+ }),
+ ScalarValue::LargeList(_) => Ok(protobuf::ScalarValue {
+ value: Some(protobuf::scalar_value::Value::LargeListValue(
+ scalar_list_value,
+ )),
+ }),
+ ScalarValue::FixedSizeList(_) => Ok(protobuf::ScalarValue {
+ value: Some(protobuf::scalar_value::Value::FixedSizeListValue(
+ scalar_list_value,
+ )),
+ }),
+ _ => unreachable!(),
+ }
+ }
+ ScalarValue::FixedSizeList(arr) => {
Review Comment:
Same for this
--
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::{List, LargeList, FixedSizedList}` to take specific types rather than `ArrayRef` [arrow-datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #8562:
URL: https://github.com/apache/arrow-datafusion/pull/8562#issuecomment-1883753783
I merged up from main to resolve a merge conflict. Thank you everyone
--
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::{List, LargeList, FixedSizedList}` to take specific types rather than `ArrayRef` [arrow-datafusion]
Posted by "rspears74 (via GitHub)" <gi...@apache.org>.
rspears74 commented on code in PR #8562:
URL: https://github.com/apache/arrow-datafusion/pull/8562#discussion_r1430356038
##########
datafusion/common/src/scalar.rs:
##########
@@ -3300,33 +3409,6 @@ mod tests {
assert_eq!(result, &expected);
}
- #[test]
Review Comment:
I removed it because it was comparing a `ListArray` with a `LargeListArray`, and thus was no longer compatible with `ScalarValue::List`. But I can add this test back and just do two `ListArray`s?
--
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::{List, LargeList, FixedSizedList}` to take specific types rather than `ArrayRef` [arrow-datafusion]
Posted by "rspears74 (via GitHub)" <gi...@apache.org>.
rspears74 commented on code in PR #8562:
URL: https://github.com/apache/arrow-datafusion/pull/8562#discussion_r1443800597
##########
datafusion/common/src/scalar.rs:
##########
@@ -2937,13 +3015,33 @@ impl fmt::Display for ScalarValue {
)?,
None => write!(f, "NULL")?,
},
- ScalarValue::List(arr)
- | ScalarValue::LargeList(arr)
- | ScalarValue::FixedSizeList(arr) => {
+ ScalarValue::List(arr) => {
// ScalarValue List should always have a single element
assert_eq!(arr.len(), 1);
let options = FormatOptions::default().with_display_error(true);
- let formatter = ArrayFormatter::try_new(arr, &options).unwrap();
+ let formatter =
Review Comment:
I'll take care of this one too. I went through my changes looking for any larger duplication sections but missed 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::{List, LargeList, FixedSizedList}` to take specific types rather than `ArrayRef` [arrow-datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #8562:
URL: https://github.com/apache/arrow-datafusion/pull/8562#discussion_r1443768896
##########
datafusion/common/src/scalar.rs:
##########
@@ -2937,13 +3015,33 @@ impl fmt::Display for ScalarValue {
)?,
None => write!(f, "NULL")?,
},
- ScalarValue::List(arr)
- | ScalarValue::LargeList(arr)
- | ScalarValue::FixedSizeList(arr) => {
+ ScalarValue::List(arr) => {
// ScalarValue List should always have a single element
assert_eq!(arr.len(), 1);
let options = FormatOptions::default().with_display_error(true);
- let formatter = ArrayFormatter::try_new(arr, &options).unwrap();
+ let formatter =
Review Comment:
The same pattern could be applied here as in https://github.com/rspears74/arrow-datafusion/pull/1 (use `first_array_for_list`)
##########
datafusion/common/src/scalar.rs:
##########
@@ -359,29 +359,77 @@ impl PartialOrd for ScalarValue {
(FixedSizeBinary(_, _), _) => None,
(LargeBinary(v1), LargeBinary(v2)) => v1.partial_cmp(v2),
(LargeBinary(_), _) => None,
- (List(arr1), List(arr2))
- | (FixedSizeList(arr1), FixedSizeList(arr2))
- | (LargeList(arr1), LargeList(arr2)) => {
- // ScalarValue::List / ScalarValue::FixedSizeList / ScalarValue::LargeList are ensure to have length 1
+ // ScalarValue::List / ScalarValue::FixedSizeList / ScalarValue::LargeList are ensure to have length 1
+ (List(arr1), List(arr2)) => {
assert_eq!(arr1.len(), 1);
assert_eq!(arr2.len(), 1);
if arr1.data_type() != arr2.data_type() {
return None;
}
- fn first_array_for_list(arr: &ArrayRef) -> ArrayRef {
- if let Some(arr) = arr.as_list_opt::<i32>() {
- arr.value(0)
- } else if let Some(arr) = arr.as_list_opt::<i64>() {
- arr.value(0)
- } else if let Some(arr) = arr.as_fixed_size_list_opt() {
- arr.value(0)
- } else {
- unreachable!("Since only List / LargeList / FixedSizeList are supported, this should never happen")
+ fn first_array_for_list(arr: &Arc<ListArray>) -> ArrayRef {
Review Comment:
Here is a proposal of how to do it: https://github.com/rspears74/arrow-datafusion/pull/1 (that targets 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::{List, LargeList, FixedSizedList}` to take specific types rather than `ArrayRef` [arrow-datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #8562:
URL: https://github.com/apache/arrow-datafusion/pull/8562#issuecomment-1877839443
I will try and find time to review this PR later this week . Sorry for the delay
--
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::{List, LargeList, FixedSizedList}` to take specific types rather than `ArrayRef` [arrow-datafusion]
Posted by "rspears74 (via GitHub)" <gi...@apache.org>.
rspears74 commented on PR #8562:
URL: https://github.com/apache/arrow-datafusion/pull/8562#issuecomment-1879574562
> Welp, I had a merge conflict, and upon fixing it, there's some new code that's incompatible with these changes (not compiling), so I'm gonna have to do a little more involved work to fix those. Will try to make that happen ASAP.
Alright that wasn't so bad. Fixed and passing tests.
--
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::{List, LargeList, FixedSizedList}` to take specific types rather than `ArrayRef` [arrow-datafusion]
Posted by "rspears74 (via GitHub)" <gi...@apache.org>.
rspears74 commented on PR #8562:
URL: https://github.com/apache/arrow-datafusion/pull/8562#issuecomment-1883835886
Thanks @alamb @jayzhan211 @wjones127 for working through this with me!
--
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::{List, LargeList, FixedSizedList}` to take specific types rather than `ArrayRef` [arrow-datafusion]
Posted by "rspears74 (via GitHub)" <gi...@apache.org>.
rspears74 commented on code in PR #8562:
URL: https://github.com/apache/arrow-datafusion/pull/8562#discussion_r1443528261
##########
datafusion/common/src/scalar.rs:
##########
@@ -359,29 +359,77 @@ impl PartialOrd for ScalarValue {
(FixedSizeBinary(_, _), _) => None,
(LargeBinary(v1), LargeBinary(v2)) => v1.partial_cmp(v2),
(LargeBinary(_), _) => None,
- (List(arr1), List(arr2))
- | (FixedSizeList(arr1), FixedSizeList(arr2))
- | (LargeList(arr1), LargeList(arr2)) => {
- // ScalarValue::List / ScalarValue::FixedSizeList / ScalarValue::LargeList are ensure to have length 1
+ // ScalarValue::List / ScalarValue::FixedSizeList / ScalarValue::LargeList are ensure to have length 1
+ (List(arr1), List(arr2)) => {
assert_eq!(arr1.len(), 1);
assert_eq!(arr2.len(), 1);
if arr1.data_type() != arr2.data_type() {
return None;
}
- fn first_array_for_list(arr: &ArrayRef) -> ArrayRef {
- if let Some(arr) = arr.as_list_opt::<i32>() {
- arr.value(0)
- } else if let Some(arr) = arr.as_list_opt::<i64>() {
- arr.value(0)
- } else if let Some(arr) = arr.as_fixed_size_list_opt() {
- arr.value(0)
- } else {
- unreachable!("Since only List / LargeList / FixedSizeList are supported, this should never happen")
+ fn first_array_for_list(arr: &Arc<ListArray>) -> ArrayRef {
Review Comment:
This one is a little tougher, because if the args to the function are "generic" (i.e. ArrayRef instead of a specific `Arc<ListArray>` type), the `arr1.value(0)` method is not available. The `first_array_for_list` function needs to be able to convert the `ArrayRef` into the specific type to then get the value out. So I'm struggling a bit coming up with a way here.
--
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::{List, LargeList, FixedSizedList}` to take specific types rather than `ArrayRef` [arrow-datafusion]
Posted by "rspears74 (via GitHub)" <gi...@apache.org>.
rspears74 commented on PR #8562:
URL: https://github.com/apache/arrow-datafusion/pull/8562#issuecomment-1879462609
Welp, I had a merge conflict, and upon fixing it, there's some new code that's incompatible with these changes, so I'm gonna have to do a little more involved work to fix those. Will try to make that happen ASAP.
--
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::{List, LargeList, FixedSizedList}` to take specific types rather than `ArrayRef` [arrow-datafusion]
Posted by "wjones127 (via GitHub)" <gi...@apache.org>.
wjones127 commented on code in PR #8562:
URL: https://github.com/apache/arrow-datafusion/pull/8562#discussion_r1443855691
##########
datafusion/common/src/scalar.rs:
##########
@@ -942,9 +958,9 @@ impl ScalarValue {
ScalarValue::Binary(_) => DataType::Binary,
ScalarValue::FixedSizeBinary(sz, _) => DataType::FixedSizeBinary(*sz),
ScalarValue::LargeBinary(_) => DataType::LargeBinary,
- ScalarValue::List(arr)
- | ScalarValue::LargeList(arr)
- | ScalarValue::FixedSizeList(arr) => arr.data_type().to_owned(),
+ ScalarValue::List(arr) => arr.data_type().to_owned(),
+ ScalarValue::LargeList(arr) => arr.data_type().to_owned(),
+ ScalarValue::FixedSizeList(arr) => arr.data_type().to_owned(),
Review Comment:
This seems like a unnecessary change.
##########
datafusion/common/src/scalar.rs:
##########
@@ -2433,11 +2455,14 @@ impl ScalarValue {
ScalarValue::LargeBinary(val) => {
eq_array_primitive!(array, index, LargeBinaryArray, val)?
}
- ScalarValue::List(arr)
- | ScalarValue::LargeList(arr)
- | ScalarValue::FixedSizeList(arr) => {
- let right = array.slice(index, 1);
- arr == &right
+ ScalarValue::List(arr) => {
+ Self::eq_array_list(&(arr.to_owned() as ArrayRef), array, index)
+ }
+ ScalarValue::LargeList(arr) => {
+ Self::eq_array_list(&(arr.to_owned() as ArrayRef), array, index)
+ }
+ ScalarValue::FixedSizeList(arr) => {
+ Self::eq_array_list(&(arr.to_owned() as ArrayRef), array, index)
Review Comment:
nit: this could be combined to avoid repeated code. (This is the last comment I'll make about this, but it applies to several more parts of your changeset)
```suggestion
ScalarValue::List(arr)
| ScalarValue::LargeList(arr)
| ScalarValue::FixedSizeList(arr) => {
Self::eq_array_list(&(arr.to_owned() as ArrayRef), array, index)
```
##########
datafusion/common/src/scalar.rs:
##########
@@ -1147,9 +1163,9 @@ impl ScalarValue {
ScalarValue::LargeBinary(v) => v.is_none(),
// arr.len() should be 1 for a list scalar, but we don't seem to
// enforce that anywhere, so we still check against array length.
- ScalarValue::List(arr)
- | ScalarValue::LargeList(arr)
- | ScalarValue::FixedSizeList(arr) => arr.len() == arr.null_count(),
+ ScalarValue::List(arr) => arr.len() == arr.null_count(),
+ ScalarValue::LargeList(arr) => arr.len() == arr.null_count(),
+ ScalarValue::FixedSizeList(arr) => arr.len() == arr.null_count(),
Review Comment:
Similar unnecessary change here.
##########
datafusion/common/src/scalar.rs:
##########
@@ -142,13 +143,13 @@ pub enum ScalarValue {
/// Fixed size list scalar.
///
/// The array must be a FixedSizeListArray with length 1.
- FixedSizeList(ArrayRef),
+ FixedSizeList(Arc<FixedSizeListArray>),
Review Comment:
I know I'm coming in a little late, but have we considered instead making the type `Scalar<Arc<FixedSizeListArray>>`? ([Scalar being the one in arrow-rs](https://docs.rs/arrow/latest/arrow/array/struct.Scalar.html)) This would enforce that the length is 1, which might be nice. I could also understand if that makes it too awkward to work with.
--
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::{List, LargeList, FixedSizedList}` to take specific types rather than `ArrayRef` [arrow-datafusion]
Posted by "rspears74 (via GitHub)" <gi...@apache.org>.
rspears74 commented on code in PR #8562:
URL: https://github.com/apache/arrow-datafusion/pull/8562#discussion_r1443859383
##########
datafusion/common/src/scalar.rs:
##########
@@ -2433,11 +2455,14 @@ impl ScalarValue {
ScalarValue::LargeBinary(val) => {
eq_array_primitive!(array, index, LargeBinaryArray, val)?
}
- ScalarValue::List(arr)
- | ScalarValue::LargeList(arr)
- | ScalarValue::FixedSizeList(arr) => {
- let right = array.slice(index, 1);
- arr == &right
+ ScalarValue::List(arr) => {
+ Self::eq_array_list(&(arr.to_owned() as ArrayRef), array, index)
+ }
+ ScalarValue::LargeList(arr) => {
+ Self::eq_array_list(&(arr.to_owned() as ArrayRef), array, index)
+ }
+ ScalarValue::FixedSizeList(arr) => {
+ Self::eq_array_list(&(arr.to_owned() as ArrayRef), array, index)
Review Comment:
Yeah, I tried this, but the args to `List`, `LargeList`, and `FixedSizeList` are all different, so they can't be put in the same match arm. This applies to all 3 comments.
--
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::{List, LargeList, FixedSizedList}` to take specific types rather than `ArrayRef` [arrow-datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on code in PR #8562:
URL: https://github.com/apache/arrow-datafusion/pull/8562#discussion_r1443880303
##########
datafusion/common/src/scalar.rs:
##########
@@ -142,13 +143,13 @@ pub enum ScalarValue {
/// Fixed size list scalar.
///
/// The array must be a FixedSizeListArray with length 1.
- FixedSizeList(ArrayRef),
+ FixedSizeList(Arc<FixedSizeListArray>),
Review Comment:
I think it is a good 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::{List, LargeList, FixedSizedList}` to take specific types rather than `ArrayRef` [arrow-datafusion]
Posted by "wjones127 (via GitHub)" <gi...@apache.org>.
wjones127 commented on code in PR #8562:
URL: https://github.com/apache/arrow-datafusion/pull/8562#discussion_r1443869268
##########
datafusion/common/src/scalar.rs:
##########
@@ -2433,11 +2455,14 @@ impl ScalarValue {
ScalarValue::LargeBinary(val) => {
eq_array_primitive!(array, index, LargeBinaryArray, val)?
}
- ScalarValue::List(arr)
- | ScalarValue::LargeList(arr)
- | ScalarValue::FixedSizeList(arr) => {
- let right = array.slice(index, 1);
- arr == &right
+ ScalarValue::List(arr) => {
+ Self::eq_array_list(&(arr.to_owned() as ArrayRef), array, index)
+ }
+ ScalarValue::LargeList(arr) => {
+ Self::eq_array_list(&(arr.to_owned() as ArrayRef), array, index)
+ }
+ ScalarValue::FixedSizeList(arr) => {
+ Self::eq_array_list(&(arr.to_owned() as ArrayRef), array, index)
Review Comment:
Ah I see now. They are now distinct data types. Nevermind.
--
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::{List, LargeList, FixedSizedList}` to take specific types rather than `ArrayRef` [arrow-datafusion]
Posted by "Weijun-H (via GitHub)" <gi...@apache.org>.
Weijun-H commented on code in PR #8562:
URL: https://github.com/apache/arrow-datafusion/pull/8562#discussion_r1441237668
##########
datafusion/common/src/scalar.rs:
##########
@@ -2936,13 +3026,33 @@ impl fmt::Display for ScalarValue {
)?,
None => write!(f, "NULL")?,
},
- ScalarValue::List(arr)
- | ScalarValue::LargeList(arr)
- | ScalarValue::FixedSizeList(arr) => {
+ ScalarValue::List(arr) => {
+ // ScalarValue List should always have a single element
+ assert_eq!(arr.len(), 1);
+ let options = FormatOptions::default().with_display_error(true);
+ let formatter =
+ ArrayFormatter::try_new(arr.as_ref() as &dyn Array, &options)
+ .unwrap();
+ let value_formatter = formatter.value(0);
+ write!(f, "{value_formatter}")?
+ }
+ ScalarValue::LargeList(arr) => {
+ // ScalarValue List should always have a single element
+ assert_eq!(arr.len(), 1);
+ let options = FormatOptions::default().with_display_error(true);
+ let formatter =
+ ArrayFormatter::try_new(arr.as_ref() as &dyn Array, &options)
+ .unwrap();
+ let value_formatter = formatter.value(0);
+ write!(f, "{value_formatter}")?
+ }
+ ScalarValue::FixedSizeList(arr) => {
// ScalarValue List should always have a single element
Review Comment:
```suggestion
// ScalarValue FixedSizeList should always have a single element
```
##########
datafusion/common/src/scalar.rs:
##########
@@ -2936,13 +3026,33 @@ impl fmt::Display for ScalarValue {
)?,
None => write!(f, "NULL")?,
},
- ScalarValue::List(arr)
- | ScalarValue::LargeList(arr)
- | ScalarValue::FixedSizeList(arr) => {
+ ScalarValue::List(arr) => {
+ // ScalarValue List should always have a single element
+ assert_eq!(arr.len(), 1);
+ let options = FormatOptions::default().with_display_error(true);
+ let formatter =
+ ArrayFormatter::try_new(arr.as_ref() as &dyn Array, &options)
+ .unwrap();
+ let value_formatter = formatter.value(0);
+ write!(f, "{value_formatter}")?
+ }
+ ScalarValue::LargeList(arr) => {
+ // ScalarValue List should always have a single element
Review Comment:
```suggestion
// ScalarValue LargeList should always have a single element
```
##########
datafusion/proto/src/logical_plan/to_proto.rs:
##########
@@ -1189,24 +1188,79 @@ impl TryFrom<&ScalarValue> for protobuf::ScalarValue {
schema: Some(schema),
};
- match val {
- ScalarValue::List(_) => Ok(protobuf::ScalarValue {
- value: Some(protobuf::scalar_value::Value::ListValue(
- scalar_list_value,
- )),
- }),
- ScalarValue::LargeList(_) => Ok(protobuf::ScalarValue {
- value: Some(protobuf::scalar_value::Value::LargeListValue(
- scalar_list_value,
- )),
- }),
- ScalarValue::FixedSizeList(_) => Ok(protobuf::ScalarValue {
- value: Some(protobuf::scalar_value::Value::FixedSizeListValue(
- scalar_list_value,
- )),
- }),
- _ => unreachable!(),
- }
+ Ok(protobuf::ScalarValue {
+ value: Some(protobuf::scalar_value::Value::ListValue(
+ scalar_list_value,
+ )),
+ })
+ }
+ ScalarValue::LargeList(arr) => {
+ // Wrap in a "field_name" column
+ let batch = RecordBatch::try_from_iter(vec![(
+ "field_name",
+ arr.to_owned() as ArrayRef,
+ )])
+ .map_err(|e| {
+ Error::General( format!("Error creating temporary batch while encoding ScalarValue::List: {e}"))
+ })?;
+
+ let gen = IpcDataGenerator {};
+ let mut dict_tracker = DictionaryTracker::new(false);
+ let (_, encoded_message) = gen
+ .encoded_batch(&batch, &mut dict_tracker, &Default::default())
+ .map_err(|e| {
+ Error::General(format!(
+ "Error encoding ScalarValue::List as IPC: {e}"
+ ))
+ })?;
+
+ let schema: protobuf::Schema = batch.schema().try_into()?;
+
+ let scalar_list_value = protobuf::ScalarListValue {
+ ipc_message: encoded_message.ipc_message,
+ arrow_data: encoded_message.arrow_data,
+ schema: Some(schema),
+ };
+
+ Ok(protobuf::ScalarValue {
+ value: Some(protobuf::scalar_value::Value::LargeListValue(
+ scalar_list_value,
+ )),
+ })
+ }
+ ScalarValue::FixedSizeList(arr) => {
+ // Wrap in a "field_name" column
+ let batch = RecordBatch::try_from_iter(vec![(
+ "field_name",
+ arr.to_owned() as ArrayRef,
+ )])
+ .map_err(|e| {
+ Error::General( format!("Error creating temporary batch while encoding ScalarValue::List: {e}"))
Review Comment:
```suggestion
Error::General( format!("Error creating temporary batch while encoding ScalarValue::FixedSizeList: {e}"))
```
##########
datafusion/proto/src/logical_plan/to_proto.rs:
##########
@@ -1189,24 +1188,79 @@ impl TryFrom<&ScalarValue> for protobuf::ScalarValue {
schema: Some(schema),
};
- match val {
- ScalarValue::List(_) => Ok(protobuf::ScalarValue {
- value: Some(protobuf::scalar_value::Value::ListValue(
- scalar_list_value,
- )),
- }),
- ScalarValue::LargeList(_) => Ok(protobuf::ScalarValue {
- value: Some(protobuf::scalar_value::Value::LargeListValue(
- scalar_list_value,
- )),
- }),
- ScalarValue::FixedSizeList(_) => Ok(protobuf::ScalarValue {
- value: Some(protobuf::scalar_value::Value::FixedSizeListValue(
- scalar_list_value,
- )),
- }),
- _ => unreachable!(),
- }
+ Ok(protobuf::ScalarValue {
+ value: Some(protobuf::scalar_value::Value::ListValue(
+ scalar_list_value,
+ )),
+ })
+ }
+ ScalarValue::LargeList(arr) => {
+ // Wrap in a "field_name" column
+ let batch = RecordBatch::try_from_iter(vec![(
+ "field_name",
+ arr.to_owned() as ArrayRef,
+ )])
+ .map_err(|e| {
+ Error::General( format!("Error creating temporary batch while encoding ScalarValue::List: {e}"))
Review Comment:
```suggestion
Error::General( format!("Error creating temporary batch while encoding ScalarValue::LargeList: {e}"))
```
--
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::{List, LargeList, FixedSizedList}` to take specific types rather than `ArrayRef` [arrow-datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #8562:
URL: https://github.com/apache/arrow-datafusion/pull/8562#discussion_r1446520763
##########
datafusion/common/src/scalar.rs:
##########
@@ -142,13 +143,13 @@ pub enum ScalarValue {
/// Fixed size list scalar.
///
/// The array must be a FixedSizeListArray with length 1.
- FixedSizeList(ArrayRef),
+ FixedSizeList(Arc<FixedSizeListArray>),
Review Comment:
Let's file a follow on ticket for continuing improving the representation of list constants
--
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