martin-g commented on code in PR #18950:
URL: https://github.com/apache/datafusion/pull/18950#discussion_r2567425490
##########
datafusion/functions/src/encoding/inner.rs:
##########
Review Comment:
FixedSizeBinary should be added here, no ?
```suggestion
LargeBinary => LargeUtf8,
FixedSizeBinary(_) => Utf8,
```
##########
datafusion/functions/src/encoding/inner.rs:
##########
@@ -553,3 +567,29 @@ fn decode(args: &[ColumnarValue]) -> Result<ColumnarValue>
{
}?;
decode_process(expression, encoding)
}
+
+#[cfg(test)]
+mod tests {
+ #[test]
+ fn test_encode_fsb() {
Review Comment:
Here are some possible tests:
```
# test for FixedSizeBinary support for encode
statement ok
CREATE TABLE test_fsb AS
SELECT arrow_cast(X'0123456789ABCDEF', 'FixedSizeBinary(8)') as fsb_col;
query TT
SELECT
encode(fsb_col, 'base64') AS fsb_base64,
encode(fsb_col, 'hex') AS fsb_hex
FROM test_fsb;
----
ASNFZ4mrze8 0123456789abcdef
# Test with NULL
query T
SELECT encode(arrow_cast(NULL, 'FixedSizeBinary(8)'), 'base64');
----
NULL
```
##########
datafusion/functions/src/encoding/inner.rs:
##########
Review Comment:
FixedSizeBinary should be added here too
```suggestion
DataType::LargeBinary => Ok(vec![DataType::LargeBinary,
DataType::Utf8]),
DataType::FixedSizeBinary(size) =>
Ok(vec\![DataType::FixedSizeBinary(*size), DataType::Utf8]),
```
##########
datafusion/functions/src/encoding/inner.rs:
##########
@@ -553,3 +570,29 @@ fn decode(args: &[ColumnarValue]) -> Result<ColumnarValue>
{
}?;
decode_process(expression, encoding)
}
+
+#[cfg(test)]
+mod tests {
+ #[test]
+ fn test_encode_fsb() {
+ use super::*;
+
+ let value = vec![0u8; 16];
+ let array =
arrow::array::FixedSizeBinaryArray::try_from_sparse_iter_with_size(
+ vec![Some(value)].into_iter(),
+ 16,
+ )
+ .unwrap();
+ let value = ColumnarValue::Array(Arc::new(array));
+
+ let ColumnarValue::Array(result) =
+ encode_process(&value, Encoding::Base64).unwrap()
+ else {
+ panic!("unexpected value");
+ };
Review Comment:
To cover the missing usage of `return_type` and `coerce_types` you need
something like:
```rust
let encode_func = EncodeFunc::new();
let args = vec![
ColumnarValue::Array(Arc::new(array)),
ColumnarValue::Scalar(ScalarValue::Utf8(Some("base64".to_string()))),
];
// This will test the full path including type checking
let result =
encode_func.invoke_with_args(datafusion_expr::ScalarFunctionArgs {
args,
number_rows: 1,
return_type: &DataType::Utf8,
}).unwrap();
let ColumnarValue::Array(result) = result else {
panic!("unexpected value");
};
...
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
