fix: stop `try_` functions from panicking by emilk · Pull Request #10730 · apache/arrow-rs

emilk · 2026-08-18T14:14:12Z

Which issue does this PR close?

Part of Proposal: all panicky functions should have a try_ version #10553
Follow-up to docs: add # Panics sections across the workspace #10656

Rationale for this change

A try_ function that returns a Result but still panics is the worst of both worlds: the caller writes error handling and gets a panic anyway.

What changes are included in this PR?

FFI_ArrowSchema::try_new: an interior nul byte in format was an unwrap, and the error path leaked the children.
MutableArrayData::try_extend: validates the array index and the start..end range, which used to underflow.
MutableArrayData::try_extend_nulls: errors instead of panicking when there is no null buffer.
VariantArray::try_value: errors on an out of bounds index, and on conflicting value/typed_value.

MutableBuffer::try_from_trusted_len_iter still panics on a bad size_hint: that is part of its unsafe contract, and the docs now say so.

Are these changes tested?

Yes, a test per error path.

Are there any user-facing changes?

Panics turn into errors. MutableArrayData::extend/extend_nulls still panic as before.

Rich-T-kid

looks mostly good to me but I have a few comments

Jefffrey · 2026-08-19T01:13:34Z

-            NullBuffer::union(a.logical_nulls().as_ref(), b.logical_nulls().as_ref()).unwrap();
+        // `null_count` is a physical count, so it can be non-zero for arrays that
+        // have no logical nulls at all:
+        let Some(nulls) = NullBuffer::union(a.logical_nulls().as_ref(), b.logical_nulls().as_ref())


do we have a reproduction for this? im struggling to conceptualize a case where we physically have nulls but logically dont have nulls 🤔

One example is RunArray… except the if above was wrong, and was checking null_count instead of is_nullable. Fixed in 97f3028

This is an actual bug fix now btw - test_try_binary_run_array_logical_nulls fails on main with

assertion `left == right` failed left: [11, 1, 31] right: [11, null, 31]

should I split out the bug fix into its own PR?

yeah we should split this into a separate PR

Extracted to fix: try_binary dropped the logical nulls of run and dictionary arrays #10748

Jefffrey · 2026-08-19T06:15:19Z

run benchmark mutable_array

adriangbot · 2026-08-19T06:19:41Z

🤖 Arrow criterion benchmark completed (GKE) | trigger

Instance: c4a-highmem-16 (12 vCPU / 65 GiB)

Comparing emilk/fix-panicking-try-fns (97f3028) to bb1e6cd (merge-base) diff

Run configuration

run benchmark mutable_array

CPU Details (lscpu)

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

group                     emilk_fix-panicking-try-fns            main
-----                     ---------------------------            ----
mutable str 1024          1.00     43.8±0.31ms        ? ?/sec    1.34     58.8±1.88ms        ? ?/sec
mutable str nulls 1024    1.00     14.2±0.11ms        ? ?/sec    1.01     14.2±0.15ms        ? ?/sec

Resource Usage

base (merge-base)

Metric	Value
Wall time	25.0s
Peak memory	136.3 MiB
Avg memory	56.6 MiB
CPU user	3.5s
CPU sys	15.3s
Peak spill	0 B

branch

Metric	Value
Wall time	25.0s
Peak memory	149.1 MiB
Avg memory	83.6 MiB
CPU user	4.4s
CPU sys	18.6s
Peak spill	0 B

File an issue against this benchmark runner

A `try_` function that returns a `Result` but still panics is the worst of both worlds. Turn those panics into errors: * `FFI_ArrowSchema::try_new`: an interior nul byte in `format` was an `unwrap`. It now returns `ArrowError::CDataInterface`, and the format is converted before the children are leaked, so the error path no longer leaks memory. * `MutableArrayData::try_extend`: validate the source array index and the `start..end` range. `end < start` used to underflow. * `MutableArrayData::try_extend_nulls`: return an error instead of panicking when the builder has no null buffer. * `VariantArray::try_value`: return an error for an out of bounds index and for a row with conflicting `value` and `typed_value`. * `try_binary`: `NullBuffer::union` returns `None` when neither input has logical nulls, which is possible even when `null_count() > 0`. Fall back to the no-nulls path instead of unwrapping. `MutableBuffer::try_from_trusted_len_iter` still panics on a bad `size_hint`: that is part of its `unsafe` contract, and the docs now say so. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Review feedback: hovering the function should show the reasons, without having to open the `try_` version's docs. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Review feedback. `FFI_ArrowSchema::try_new` had the reason in prose, `try_binary` used `# Error`, and `try_from_trusted_len_iter` did not say what its `Err` meant. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

emilk · 2026-08-19T06:41:56Z

Split out fix: try_binary dropped the logical nulls of run and dictionary arrays #10748 from this PR

github-actions Bot added arrow Changes to the arrow crate parquet-variant parquet-variant* crates arrow-schema arrow-data arrow-buffer arrow-arith labels Aug 18, 2026

emilk marked this pull request as ready for review August 18, 2026 14:16

emilk mentioned this pull request Aug 18, 2026

Rerun PRs rerun-io/opensource#2

Open

Rich-T-kid suggested changes Aug 18, 2026

View reviewed changes

Comment thread arrow-schema/src/ffi.rs

Comment thread parquet-variant-compute/src/variant_array.rs Outdated

Comment thread arrow-data/src/transform/mod.rs Outdated

Comment thread arrow-data/src/transform/mod.rs Outdated

Jefffrey reviewed Aug 19, 2026

View reviewed changes

Jefffrey added the bug label Aug 19, 2026

emilk force-pushed the emilk/fix-panicking-try-fns branch 2 times, most recently from 4659507 to 97f3028 Compare August 19, 2026 03:16

This comment was marked as duplicate.

Sign in to view

emilk and others added 3 commits August 19, 2026 08:38

docs: spell out the panic reasons instead of cross-referencing

9072fa2

Review feedback: hovering the function should show the reasons, without having to open the `try_` version's docs. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

docs: give the try_ functions an # Errors section

b0c7b97

Review feedback. `FFI_ArrowSchema::try_new` had the reason in prose, `try_binary` used `# Error`, and `try_from_trusted_len_iter` did not say what its `Err` meant. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

emilk force-pushed the emilk/fix-panicking-try-fns branch from 97f3028 to b0c7b97 Compare August 19, 2026 06:40

emilk mentioned this pull request Aug 19, 2026

fix: try_binary dropped the logical nulls of run and dictionary arrays #10748

Open

Jefffrey approved these changes Aug 19, 2026

View reviewed changes

Navigation Menu

Uh oh!

fix: stop `try_` functions from panicking - #10730

fix: stop `try_` functions from panicking#10730
emilk wants to merge 3 commits into
apache:mainfrom
emilk:emilk/fix-panicking-try-fns

emilk commented Aug 18, 2026 •

edited

Loading

Uh oh!

Rich-T-kid left a comment •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Jefffrey Aug 19, 2026

Uh oh!

emilk Aug 19, 2026

Uh oh!

emilk Aug 19, 2026 •

edited

Loading

Uh oh!

Jefffrey Aug 19, 2026

Uh oh!

emilk Aug 19, 2026

Uh oh!

Jefffrey commented Aug 19, 2026

Uh oh!

This comment was marked as duplicate.

adriangbot commented Aug 19, 2026

Uh oh!

emilk commented Aug 19, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

emilk commented Aug 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

Rich-T-kid left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Jefffrey Aug 19, 2026

Choose a reason for hiding this comment

Uh oh!

emilk Aug 19, 2026

Choose a reason for hiding this comment

Uh oh!

emilk Aug 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Jefffrey Aug 19, 2026

Choose a reason for hiding this comment

Uh oh!

emilk Aug 19, 2026

Choose a reason for hiding this comment

Uh oh!

Jefffrey commented Aug 19, 2026

Uh oh!

This comment was marked as duplicate.

adriangbot commented Aug 19, 2026

Uh oh!

emilk commented Aug 19, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

emilk commented Aug 18, 2026 •

edited

Loading

Rich-T-kid left a comment •

edited

Loading

emilk Aug 19, 2026 •

edited

Loading