Implement ScalarValue for FixedSizeBinary #3911
Conversation
|
This is dependent on apache/arrow-rs#2905 |
alamb
added
the
waiting-on-upstream
datafusion/proto/src/to_proto.rs
Outdated
| @@ -1187,6 +1187,7 @@ impl TryFrom<&ScalarValue> for protobuf::ScalarValue { | |||
| Value::LargeBinaryValue(s.to_owned()) | |||
| }) | |||
| } | |||
| scalar::ScalarValue::FixedSizeBinary(_, _) => todo!(), | |||
There was a problem hiding this comment.
Could you please return a "NotImplemented" error here rather than a todo() panic?
There was a problem hiding this comment.
Done. The error type in to_proto doesn't have the NotImplented variant as it isn't a DataFusionError, hopefully a string saying that it isn't implemented is sufficient?
I added some tests. I may have been mistaking the dependency on arrow-rs/2905 for some other thing that was exploding at the same time in our code while implementing this. It does seem like basic ordering and grouping operations work without it. |
| @@ -48,6 +48,28 @@ async fn parquet_query() { | |||
| assert_batches_eq!(expected, &actual); | |||
| } | |||
|
|
|||
| #[tokio::test] | |||
| async fn fixed_size_binary_columns() { | |||
| @@ -1187,6 +1187,11 @@ impl TryFrom<&ScalarValue> for protobuf::ScalarValue { | |||
| Value::LargeBinaryValue(s.to_owned()) | |||
| }) | |||
| } | |||
| scalar::ScalarValue::FixedSizeBinary(_, _) => { | |||
* implement ScalarValue for FixedSizeBinary * Return an error instead of panicking * add tests for scalar value fixedsizebinary variant
Which issue does this PR close?
Closes #3910
Rationale for this change
To be able to use FixedSizeBinary for sorting / grouping, we need to implement a ScalarValue variant for this type.