Rich-T-kid commented on code in PR #10324:
URL: https://github.com/apache/arrow-rs/pull/10324#discussion_r3567201242
##########
arrow-cast/src/base64.rs:
##########
@@ -49,10 +53,8 @@ pub fn b64_encode<E: Engine, O: OffsetSizeTrait>(
}
assert_eq!(offset, buffer_len);
- // Safety: Base64 is valid UTF-8
- unsafe {
- GenericStringArray::new_unchecked(offsets, Buffer::from_vec(buffer),
array.nulls().cloned())
- }
+ // `try_new` validates UTF-8 instead of trusting the (safe-trait) Engine.
+ GenericStringArray::try_new(offsets, Buffer::from_vec(buffer),
array.nulls().cloned())
Review Comment:
I think it makes sense to have either an alternate function or passing in a
boolean for utf8 validation.
I don't think its should be `arrow-rs` responsibility to validate the engine
by default, especially when it comes with performance penalty.
maybe something similar to
```rust
pub fn b64_encode<E: Engine, O: OffsetSizeTrait>(
engine: &E,
array: &GenericBinaryArray<O>,
validate_utf8: boolean,
)
```
and
```rust
switch validate_utf8 {
true => GenericStringArray::try_new(offsets, Buffer::from_vec(buffer),
array.nulls().cloned())
false => GenericStringArray::new_unchecked(offsets,
Buffer::from_vec(buffer), array.nulls().cloned())
}
```
to avoid introducing an breaking API change it may make sense to make a
separate function that handles the validation.
```rust
pub fn b64_encode_validate<E: Engine, O: OffsetSizeTrait>(
engine: &E,
array: &GenericBinaryArray<O>,
)
```
--
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]
