mbutrovich commented on code in PR #26377:
URL: https://github.com/apache/datafusion/pull/26377#discussion_r4286031936
##########
datafusion/functions/src/strings.rs:
##########
@@ -1753,16 +1796,27 @@ mod tests {
#[test]
fn string_view_builder_try_append_success_path() {
- let mut builder = StringViewArrayBuilder::with_capacity(3);
+ let mut builder = StringViewArrayBuilder::with_capacity(5);
builder.try_append_value("abc").unwrap();
builder.try_append_placeholder().unwrap();
builder.try_append_value("a long string value").unwrap();
+ // SAFETY: ASCII inputs and ASCII outputs.
+ unsafe {
+ builder
+ .try_append_byte_map(b"de", |b| b.to_ascii_uppercase())
+ .unwrap();
+ builder
+ .try_append_byte_map(b"a long mapped value", |b|
b.to_ascii_uppercase())
+ .unwrap();
+ }
- let nulls = Some(NullBuffer::from(vec![true, false, true]));
+ let nulls = Some(NullBuffer::from(vec![true, false, true, true,
true]));
Review Comment:
Could this test also check the view layout? `array.value(i)` reads a long
view through its offset and buffer index and never looks at the prefix. So
nothing here checks the invariant noted at line 877 that the prefix comes from
the mapped bytes. As a check, I changed the `make_long_view_checked` call in
`try_append_byte_map` to pass `src` instead of `&self.in_progress[cursor..]`.
All of `cargo test -p datafusion-functions --lib` still passed, and so did the
`upper` / `lower` queries in `functions.slt`. With the line below added, this
test fails with `Mismatch between embedded prefix and data`.
[`validate_full`](https://github.com/apache/arrow-rs/blob/ef1fa157977633f0ba21aa921f9ef3d5c669235b/arrow-data/src/byte_view.rs#L170-L201)
also checks the zero padding of inline views, which covers the new inline
construction at line 852.
```suggestion
let array = builder.finish(nulls).unwrap();
array.to_data().validate_full().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: [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]
