You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "scovich (via GitHub)" <gi...@apache.org> on 2025/08/20 22:55:26 UTC
[PR] [Variant] VariantArrayBuilder tracks only starting offsets [arrow-rs]
scovich opened a new pull request, #8193:
URL: https://github.com/apache/arrow-rs/pull/8193
# Which issue does this PR close?
- Closes https://github.com/apache/arrow-rs/issues/8192
# Rationale for this change
Tracking lengths is redundant because that's just the difference between adjacent offsets. It becomes even easier if we eventually start adding the views eagerly instead of storing up offsets, because we only need to remember the difference between starting and final offset.
# What changes are included in this PR?
Update the algorithm that creates the binary view to compute the offsets.
# Are these changes tested?
Existing tests cover this change.
# Are there any user-facing changes?
No.
--
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] [Variant] VariantArrayBuilder tracks only offsets [arrow-rs]
Posted by "klion26 (via GitHub)" <gi...@apache.org>.
klion26 commented on code in PR #8193:
URL: https://github.com/apache/arrow-rs/pull/8193#discussion_r2290493577
##########
parquet-variant-compute/src/variant_array_builder.rs:
##########
@@ -338,19 +327,21 @@ impl Drop for VariantArrayVariantBuilder<'_> {
}
}
-fn binary_view_array_from_buffers(
- buffer: Vec<u8>,
- locations: Vec<(usize, usize)>,
-) -> BinaryViewArray {
- let mut builder = BinaryViewBuilder::with_capacity(locations.len());
+fn binary_view_array_from_buffers(buffer: Vec<u8>, offsets: Vec<usize>) -> BinaryViewArray {
+ // All offsets are less than or equal to the buffer length, so we can safely cast all offsets
+ // inside the loop below, as long as the buffer length fits in u32.
+ u32::try_from(buffer.len()).expect("buffer length should fit in u32");
Review Comment:
Out of curiosity: Why do we check the length of `buffer` instead of the last element of `offsets` 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] [Variant] VariantArrayBuilder tracks only offsets [arrow-rs]
Posted by "scovich (via GitHub)" <gi...@apache.org>.
scovich commented on PR #8193:
URL: https://github.com/apache/arrow-rs/pull/8193#issuecomment-3211389752
@alamb -- ready for your review+merge?
--
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] [Variant] VariantArrayBuilder tracks only offsets [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #8193:
URL: https://github.com/apache/arrow-rs/pull/8193
--
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] [Variant] VariantArrayBuilder tracks only offsets [arrow-rs]
Posted by "scovich (via GitHub)" <gi...@apache.org>.
scovich commented on code in PR #8193:
URL: https://github.com/apache/arrow-rs/pull/8193#discussion_r2291056832
##########
parquet-variant-compute/src/variant_array_builder.rs:
##########
@@ -338,19 +327,21 @@ impl Drop for VariantArrayVariantBuilder<'_> {
}
}
-fn binary_view_array_from_buffers(
- buffer: Vec<u8>,
- locations: Vec<(usize, usize)>,
-) -> BinaryViewArray {
- let mut builder = BinaryViewBuilder::with_capacity(locations.len());
+fn binary_view_array_from_buffers(buffer: Vec<u8>, offsets: Vec<usize>) -> BinaryViewArray {
+ // All offsets are less than or equal to the buffer length, so we can safely cast all offsets
+ // inside the loop below, as long as the buffer length fits in u32.
+ u32::try_from(buffer.len()).expect("buffer length should fit in u32");
Review Comment:
No offset can legally be larger than the buffer's length -- the binary view would reject it -- and indeed the last offset _is_ the buffer's length. It seemed simpler to just use that fact, which works even when `offsets.is_empty()`, rather than have to worry about (and document) the possibly-empty case:
```rust
if let Some(offset) = offsets.last() {
u32::try_from(offset).expect(...);
}
```
--
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