You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "Cintu07 (via GitHub)" <gi...@apache.org> on 2026/09/05 04:06:53 UTC
[PR] fix(arrow-string): saturate the offset casts in substring [arrow-rs]
Cintu07 opened a new pull request, #10995:
URL: https://github.com/apache/arrow-rs/pull/10995
# Which issue does this PR close?
- Closes #10983.
# Rationale for this change
`substring` takes `start: i64` and `length: Option<u64>` and cast both straight into the offset type. on the `Utf8` and `Binary` arms a start at or above 2^31 wrapped negative, and `byte_substring` reads a negative start as counting from the end of the value, so the call did not fail, it quietly did something else. the `LargeUtf8` and `LargeBinary` arms narrow to i64 and were not affected, so the same call returned different data depending only on the offset width of the input.
saturating rather than rejecting, per the discussion on the issue. a start past the end of every value is what the caller asked for, and it is already what the 64 bit arms do.
# What changes are included in this PR?
- `start` and `length` saturate into the offset type at the dispatch instead of being cast
- inside `byte_substring`, the two additions that can carry a saturated value past the offset type go through `checked_add` and clamp to the end of the value
one thing i did not expect. the `length` cast was wrong on all four arms, not only the 32 bit ones. `u64::MAX as i64` is -1, and a negative length puts the end of a substring before its start, which drives the output offsets negative and then allocates on the result of `as_usize`. i only found that because the test compares the narrow and wide arms against each other, and the wide one panicked. so `LargeUtf8` and `LargeBinary` are fixed here too.
the third addition, `pair[1] + start` on the negative branch, is left alone. `pair[1]` is non-negative and `start` is at worst `i32::MIN`, so it cannot overflow.
# Are these changes tested?
yes. `out_of_range_start_and_length_match_the_64_bit_arms` runs four out of range starts against three lengths, on `Utf8` against `LargeUtf8` and on `Binary` against `LargeBinary`, and asserts the pairs agree. it also pins the answer itself, since agreeing on the wrong result would still pass: skipping 2^31 characters of a five character string gives empty strings, and a start that already fits is untouched.
on current main that test fails twice. `Utf8` returns `["hello", "world"]` where `LargeUtf8` returns `["", ""]`, and `Some(u64::MAX)` panics inside `MutableBuffer`.
`arrow-string` is 189 passed, and fmt and clippy with `-D warnings` are clean.
# Are there any user-facing changes?
yes, for input that was previously wrong. a `start` or `length` outside the offset type now saturates, so `Utf8` and `Binary` return what `LargeUtf8` and `LargeBinary` already returned. values that fit are unaffected.
--
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] fix(arrow-string): saturate the offset casts in substring [arrow-rs]
Posted by "adriangbot (via GitHub)" <gi...@apache.org>.
adriangbot commented on PR #10995:
URL: https://github.com/apache/arrow-rs/pull/10995#issuecomment-5556410545
🤖 Arrow criterion benchmark running (GKE) | [trigger](https://github.com/apache/arrow-rs/pull/10995#issuecomment-5556397045)
**Instance:** `c4a-highmem-16` (12 vCPU / 65 GiB) | `Linux bench-c5556397045-2144-6699w 6.12.94+ #1 SMP Fri Jul 17 09:42:57 UTC 2026 aarch64 GNU/Linux`
<details><summary>CPU Details (lscpu)</summary>
```
Architecture: aarch64
CPU op-mode(s): 64-bit
Byte Order: Little Endian
CPU(s): 16
On-line CPU(s) list: 0-15
Vendor ID: ARM
Model name: Neoverse-V2
Model: 1
Thread(s) per core: 1
Core(s) per cluster: 16
Socket(s): -
Cluster(s): 1
Stepping: r0p1
BogoMIPS: 2000.00
Flags: fp asimd evtstrm aes pmull sha1 sha2 crc32 atomics fphp asimdhp cpuid asimdrdm jscvt fcma lrcpc dcpop sha3 sm3 sm4 asimddp sha512 sve asimdfhm dit uscat ilrcpc flagm sb paca pacg dcpodp sve2 sveaes svepmull svebitperm svesha3 svesm4 flagm2 frint svei8mm svebf16 i8mm bf16 dgh rng bti
L1d cache: 1 MiB (16 instances)
L1i cache: 1 MiB (16 instances)
L2 cache: 32 MiB (16 instances)
L3 cache: 80 MiB (1 instance)
NUMA node(s): 1
NUMA node0 CPU(s): 0-15
Vulnerability Gather data sampling: Not affected
Vulnerability Indirect target selection: Not affected
Vulnerability Itlb multihit: Not affected
Vulnerability L1tf: Not affected
Vulnerability Mds: Not affected
Vulnerability Meltdown: Not affected
Vulnerability Mmio stale data: Not affected
Vulnerability Reg file data sampling: Not affected
Vulnerability Retbleed: Not affected
Vulnerability Spec rstack overflow: Not affected
Vulnerability Spec store bypass: Mitigation; Speculative Store Bypass disabled via prctl
Vulnerability Spectre v1: Mitigation; __user pointer sanitization
Vulnerability Spectre v2: Mitigation; CSV2, BHB
Vulnerability Srbds: Not affected
Vulnerability Tsa: Not affected
Vulnerability Tsx async abort: Not affected
Vulnerability Vmscape: Not affected
```
</details>
Comparing fix/substring-saturate-offset-cast (4136775d5fff111d4a66b36cbd9c92595c800142) to b1aa94b (merge-base) [diff](https://github.com/apache/arrow-rs/compare/b1aa94b05c88ef4cc6d0293d784bbc254537e942..4136775d5fff111d4a66b36cbd9c92595c800142)
<details><summary>Run configuration</summary>
```yaml
run benchmark substring_kernels
```
</details>
BENCH_COMMAND=cargo bench --features=arrow,async,test_common,experimental,object_store --bench substring_kernels
Results will be posted here when complete
---
[File an issue](https://github.com/adriangb/datafusion-benchmarking/issues) against this benchmark runner
--
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] fix(arrow-string): saturate the offset casts in substring [arrow-rs]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on PR #10995:
URL: https://github.com/apache/arrow-rs/pull/10995#issuecomment-5556397045
run benchmark substring_kernels
--
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] fix(arrow-string): saturate the offset casts in substring [arrow-rs]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey merged PR #10995:
URL: https://github.com/apache/arrow-rs/pull/10995
--
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] fix(arrow-string): saturate the offset casts in substring [arrow-rs]
Posted by "adriangbot (via GitHub)" <gi...@apache.org>.
adriangbot commented on PR #10995:
URL: https://github.com/apache/arrow-rs/pull/10995#issuecomment-5556425792
🤖 Arrow criterion benchmark completed (GKE) | [trigger](https://github.com/apache/arrow-rs/pull/10995#issuecomment-5556397045)
**Instance:** `c4a-highmem-16` (12 vCPU / 65 GiB)
Comparing fix/substring-saturate-offset-cast (4136775d5fff111d4a66b36cbd9c92595c800142) to b1aa94b (merge-base) [diff](https://github.com/apache/arrow-rs/compare/b1aa94b05c88ef4cc6d0293d784bbc254537e942..4136775d5fff111d4a66b36cbd9c92595c800142)
<details><summary>Run configuration</summary>
```yaml
run benchmark substring_kernels
```
</details>
<details><summary>CPU Details (lscpu)</summary>
```
Architecture: aarch64
CPU op-mode(s): 64-bit
Byte Order: Little Endian
CPU(s): 16
On-line CPU(s) list: 0-15
Vendor ID: ARM
Model name: Neoverse-V2
Model: 1
Thread(s) per core: 1
Core(s) per cluster: 16
Socket(s): -
Cluster(s): 1
Stepping: r0p1
BogoMIPS: 2000.00
Flags: fp asimd evtstrm aes pmull sha1 sha2 crc32 atomics fphp asimdhp cpuid asimdrdm jscvt fcma lrcpc dcpop sha3 sm3 sm4 asimddp sha512 sve asimdfhm dit uscat ilrcpc flagm sb paca pacg dcpodp sve2 sveaes svepmull svebitperm svesha3 svesm4 flagm2 frint svei8mm svebf16 i8mm bf16 dgh rng bti
L1d cache: 1 MiB (16 instances)
L1i cache: 1 MiB (16 instances)
L2 cache: 32 MiB (16 instances)
L3 cache: 80 MiB (1 instance)
NUMA node(s): 1
NUMA node0 CPU(s): 0-15
Vulnerability Gather data sampling: Not affected
Vulnerability Indirect target selection: Not affected
Vulnerability Itlb multihit: Not affected
Vulnerability L1tf: Not affected
Vulnerability Mds: Not affected
Vulnerability Meltdown: Not affected
Vulnerability Mmio stale data: Not affected
Vulnerability Reg file data sampling: Not affected
Vulnerability Retbleed: Not affected
Vulnerability Spec rstack overflow: Not affected
Vulnerability Spec store bypass: Mitigation; Speculative Store Bypass disabled via prctl
Vulnerability Spectre v1: Mitigation; __user pointer sanitization
Vulnerability Spectre v2: Mitigation; CSV2, BHB
Vulnerability Srbds: Not affected
Vulnerability Tsa: Not affected
Vulnerability Tsx async abort: Not affected
Vulnerability Vmscape: Not affected
```
</details>
<details><summary>Details</summary>
<p>
```
group fix_substring-saturate-offset-cast main
----- ---------------------------------- ----
substring by char (ascii, prefix) 1.03 4.2±0.30ms ? ?/sec 1.00 4.0±0.22ms ? ?/sec
substring by char (ascii, tail) 1.00 4.1±0.13ms ? ?/sec 1.08 4.5±0.35ms ? ?/sec
substring by char (non-ascii, prefix) 1.01 1498.5±14.91µs ? ?/sec 1.00 1488.9±35.72µs ? ?/sec
substring by char (non-ascii, tail) 1.10 1632.5±117.14µs ? ?/sec 1.00 1486.7±17.73µs ? ?/sec
substring fixed size binary array 1.01 20.0±1.11ms ? ?/sec 1.00 19.7±0.86ms ? ?/sec
substring utf8 (start = 0, length = None) 1.22 26.4±0.90ms ? ?/sec 1.00 21.6±1.15ms ? ?/sec
substring utf8 (start = 1, length = str_len - 1) 1.08 24.7±1.45ms ? ?/sec 1.00 23.0±1.39ms ? ?/sec
substring utf8 by char 1.11 29.6±1.34ms ? ?/sec 1.00 26.7±1.09ms ? ?/sec
```
</p>
</details>
<details><summary>Resource Usage</summary>
**base (merge-base)**
| Metric | Value |
|--------|-------|
| Wall time | 90.0s |
| Peak memory | 275.6 MiB |
| Avg memory | 222.4 MiB |
| CPU user | 51.7s |
| CPU sys | 35.2s |
| Peak spill | 0 B |
**branch**
| Metric | Value |
|--------|-------|
| Wall time | 85.0s |
| Peak memory | 275.9 MiB |
| Avg memory | 220.8 MiB |
| CPU user | 50.7s |
| CPU sys | 32.4s |
| Peak spill | 0 B |
</details>
---
[File an issue](https://github.com/adriangb/datafusion-benchmarking/issues) against this benchmark runner
--
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] fix(arrow-string): saturate the offset casts in substring [arrow-rs]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on PR #10995:
URL: https://github.com/apache/arrow-rs/pull/10995#issuecomment-5758078843
thanks @Cintu07
--
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] fix(arrow-string): saturate the offset casts in substring [arrow-rs]
Posted by "Cintu07 (via GitHub)" <gi...@apache.org>.
Cintu07 commented on PR #10995:
URL: https://github.com/apache/arrow-rs/pull/10995#issuecomment-5556790460
i ran it on my machine too, aarch64.
different rows moved for me. your +22% row came out +1.8%. instead by char
(non-ascii, prefix) was +11.7%.
so i ran the same branch again against the same baseline, no code change at all
between the two runs:
by char (non-ascii, prefix) +53%
by char (non-ascii, tail) +31%
by char (ascii, prefix) -3.7% improved
by char (ascii, tail) -2.3% improved
same code both times. so this bench can swing 50% on my box by itself.
the row that runs the new branches is start = 1, length = str_len - 1. run one was
+0.06% at p 0.96, run two -1.35% at p 0.22. it did not move either time.
my laptop is noisier than your runner, sure. but every by char row goes through
substring_by_char_impl and the diff never touches that function, so those cannot be
my change.
if you would rather not have the extra branch in the loop at all, i can move the
checks out. start and length do not change per row, they only need checking once.
--
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] fix(arrow-string): saturate the offset casts in substring [arrow-rs]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on code in PR #10995:
URL: https://github.com/apache/arrow-rs/pull/10995#discussion_r4042915188
##########
arrow-string/src/substring.rs:
##########
@@ -316,6 +320,30 @@ fn binary_view_substring(
Ok(Arc::new(builder.finish()))
}
+/// Clamps a 64 bit `start` into the 32 bit offset domain.
+///
+/// A `start` past `i32::MAX` means "past the end of every value", and saturating
+/// keeps that meaning. Casting wrapped it to a negative start, which
+/// [`byte_substring`] reads as counting from the end of the value instead, so the
+/// same call returned different data on `Utf8` than on `LargeUtf8`.
+fn saturating_start_i32(start: i64) -> i32 {
+ start.clamp(i32::MIN as i64, i32::MAX as i64) as i32
+}
Review Comment:
```suggestion
fn i64_to_i32_saturating(value: i64) -> i32 {
value.clamp(i32::MIN as i64, i32::MAX as i64) as i32
}
```
##########
arrow-string/src/substring.rs:
##########
@@ -81,27 +81,31 @@ pub fn substring(
let values = substring(dictionary.values(), start, length)?;
Ok(Arc::new(dictionary.with_values(values)))
}
- DataType::LargeBinary => {
- byte_substring(array.as_binary::<i64>(), start, length.map(|e| e as i64))
- }
+ DataType::LargeBinary => byte_substring(
+ array.as_binary::<i64>(),
+ start,
+ length.map(saturating_length_i64),
Review Comment:
```suggestion
// ensure we saturate to not wrap around to a negative length
length.map(saturating_length_i64),
```
##########
arrow-string/src/substring.rs:
##########
@@ -81,27 +81,31 @@ pub fn substring(
let values = substring(dictionary.values(), start, length)?;
Ok(Arc::new(dictionary.with_values(values)))
}
- DataType::LargeBinary => {
- byte_substring(array.as_binary::<i64>(), start, length.map(|e| e as i64))
- }
+ DataType::LargeBinary => byte_substring(
+ array.as_binary::<i64>(),
+ start,
+ length.map(saturating_length_i64),
+ ),
DataType::Binary => byte_substring(
array.as_binary::<i32>(),
- start as i32,
- length.map(|e| e as i32),
+ saturating_start_i32(start),
Review Comment:
```suggestion
// ensure to saturate to avoid wrapping to negative which is a different behaviour
saturating_start_i32(start),
```
##########
arrow-string/src/substring.rs:
##########
@@ -1236,4 +1273,57 @@ mod tests {
vec![Some("hel"), Some("bye")]
);
}
+
+ /// A `start` or `length` past the 32 bit offset domain used to be cast
+ /// straight to `i32`, wrapping negative. `byte_substring` reads a negative
+ /// start as counting from the end, so the 32 bit arms silently did something
+ /// different from the 64 bit arms for the same call. They agree now.
+ #[test]
+ fn out_of_range_start_and_length_match_the_64_bit_arms() {
+ let values = vec![Some("hello"), Some("world"), None];
Review Comment:
```suggestion
#[test]
fn out_of_range_start_and_length_match_the_64_bit_arms() {
// use 64 bit offset versions as expected behaviour for extreme start & length values
// which should saturate and not wrap
let values = vec![Some("hello"), Some("world"), None];
```
##########
arrow-string/src/substring.rs:
##########
@@ -316,6 +320,30 @@ fn binary_view_substring(
Ok(Arc::new(builder.finish()))
}
+/// Clamps a 64 bit `start` into the 32 bit offset domain.
+///
+/// A `start` past `i32::MAX` means "past the end of every value", and saturating
+/// keeps that meaning. Casting wrapped it to a negative start, which
+/// [`byte_substring`] reads as counting from the end of the value instead, so the
+/// same call returned different data on `Utf8` than on `LargeUtf8`.
+fn saturating_start_i32(start: i64) -> i32 {
+ start.clamp(i32::MIN as i64, i32::MAX as i64) as i32
+}
+
+/// Clamps a `length` into the 32 bit offset domain. Lengths are unsigned, so only
+/// the upper bound can be exceeded.
+fn saturating_length_i32(length: u64) -> i32 {
+ length.min(i32::MAX as u64) as i32
+}
+
+/// Clamps a `length` into the 64 bit offset domain.
+///
+/// `u64::MAX as i64` is -1, and a negative length puts the end of the substring
+/// before its start, which drives the output offsets negative.
+fn saturating_length_i64(length: u64) -> i64 {
+ length.min(i64::MAX as u64) as i64
+}
Review Comment:
```suggestion
fn u64_to_i64_saturating(value: u64) -> i64 {
value.min(i64::MAX as u64) as i64
}
```
##########
arrow-string/src/substring.rs:
##########
@@ -316,6 +320,30 @@ fn binary_view_substring(
Ok(Arc::new(builder.finish()))
}
+/// Clamps a 64 bit `start` into the 32 bit offset domain.
+///
+/// A `start` past `i32::MAX` means "past the end of every value", and saturating
+/// keeps that meaning. Casting wrapped it to a negative start, which
+/// [`byte_substring`] reads as counting from the end of the value instead, so the
+/// same call returned different data on `Utf8` than on `LargeUtf8`.
+fn saturating_start_i32(start: i64) -> i32 {
+ start.clamp(i32::MIN as i64, i32::MAX as i64) as i32
+}
+
+/// Clamps a `length` into the 32 bit offset domain. Lengths are unsigned, so only
+/// the upper bound can be exceeded.
+fn saturating_length_i32(length: u64) -> i32 {
+ length.min(i32::MAX as u64) as i32
+}
Review Comment:
```suggestion
fn u64_to_i32_saturating(value: u64) -> i32 {
value.min(i32::MAX as u64) as i32
}
```
--
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