Skip to content

fix(rust): Centralize FixedSizeList outer-to-child layout arithmetic and fix under-reservation - #28993

Open
Angshuman09 wants to merge 3 commits into
pola-rs:mainfrom
Angshuman09:fix/fixed-size-list-reserve
Open

Angshuman09 wants to merge 3 commits into
pola-rs:mainfrom
Angshuman09:fix/fixed-size-list-reserve

Conversation

@Angshuman09

@Angshuman09 Angshuman09 commented Aug 26, 2026 •

Copy link
Copy Markdown

Closes #28979
Resolves #28872


Core Problem & Background

In Polars and Arrow, FixedSizeList operates simultaneously across two distinct coordinate spaces:

  1. Outer units (List level): Number of list items / rows (e.g., reserving space for $N$ lists, row offset $i$, slice length $L$).
  2. Child units (Inner buffer level): Number of flattened primitive elements in the underlying leaf storage (inner_builder or values), where each outer list item contains $K$ child elements ($K = \text{width} = \text{size}$).

$$\text{Child Offset} = \text{Outer Offset} \times \text{size}$$ $$\text{Child Length} = \text{Outer Length} \times \text{size}$$

Because both outer and child counts are represented as primitive usize integers, unit mismatch is invisible to the Rust compiler. Passing an outer unit where a child unit is expected compiles without warning or type error.


The Failure Mode (#28872)

In FixedSizeListArrayBuilder::reserve and MutableFixedSizeListArray::reserve:

// BEFORE:
fn reserve(&mut self, additional: usize) {
    self.inner_builder.reserve(additional); // Under-reserves child storage!
    self.validity.reserve(additional);
}

What went wrong:

  • additional is in outer list units.
  • self.validity.reserve(additional) is correct because the validity bitmap is indexed per outer list slot.
  • self.inner_builder.reserve(additional) is wrong because inner_builder stores child elements. Reserving only additional child slots instead of additional * self.size allocates only $\frac{1}{\text{size}}$ of the required storage.

Concrete Example:

For a FixedSizeList with width $K = 16$:

  • Reserving $N = 1{,}000$ outer rows asked the inner child builder to reserve only $1{,}000$ child items instead of $16{,}000$.
  • Because Rust dynamic vectors reallocate automatically on push, this bug did not produce incorrect computational results, but it completely defeated pre-allocation and forced the inner builder to repeatedly trigger expensive reallocations, memory copies, and vector resizes during appends.

Design & Solution

Instead of scattering raw outer * self.size multiplications across operations (where omissions can easily happen again), this PR centralizes all coordinate arithmetic into explicit conversion primitives.

1. Centralized Primitives

Introduced child_offset and child_length in crates/polars-arrow/src/array/fixed_size_list/mod.rs:

#[inline(always)]
pub(crate) const fn child_offset(outer_offset: usize, size: usize) -> usize {
    outer_offset * size
}

#[inline(always)]
pub(crate) const fn child_length(outer_length: usize, size: usize) -> usize {
    outer_length * size
}

2. Fixed Builders Pre-Allocation

Updated reserve in both builders to convert outer units before delegating to the child buffer:

  • FixedSizeListArrayBuilder:
    fn reserve(&mut self, additional: usize) {
        self.inner_builder.reserve(child_length(additional, self.size));
        self.validity.reserve(additional);
    }
  • MutableFixedSizeListArray:
    pub fn reserve(&mut self, additional: usize) {
        self.values.reserve(child_length(additional, self.size));
        if let Some(x) = self.validity.as_mut() {
            x.reserve(additional);
        }
    }

Verification

Added Regression Tests

  1. crates/polars/tests/it/arrow/array/fixed_size_list/mutable.rs:
    • test_reserve: Verifies that MutableFixedSizeListArray::reserve(10) with width 3 allocates at least 30 child units in the underlying buffer.
  2. crates/polars/tests/it/arrow/array/fixed_size_list/mod.rs:
    • test_builder: Tests FixedSizeListArrayBuilder end-to-end (pre-allocation with reserve, extend_nulls, subslice_extend, and freeze_reset).

Test Results

cargo test -p polars --test it fixed_size_list
Screenshot 2026-08-26 at 8 00 30 PM

@github-actions github-actions Bot added fix Bug fix rust Related to Rust Polars first-contribution First contribution by user labels Aug 26, 2026
@Angshuman09 Angshuman09 changed the title fix(rust): Fix fixed-size list builder capacity fix: Centralize FixedSizeList outer-to-child layout arithmetic and fix under-reservation Aug 26, 2026
@github-actions github-actions Bot added the python Related to Python Polars label Aug 26, 2026
@0guban0v

Copy link
Copy Markdown
Contributor

#28980 already addresses #28872 and was under maintainer review before this PR was opened. I created #28872 and #28979 as separate changes so each could have focused implementation, measurable evidence, and regression coverage. I'm unsure how overlapping PRs are handled here, so I'll leave that decision to @ritchie46

@codecov

codecov Bot commented Aug 26, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 81.08108% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.35%. Comparing base (5d8ebab) to head (9e50e37).
⚠️ Report is 129 commits behind head on main.

Files with missing lines Patch % Lines
.../polars-arrow/src/array/fixed_size_list/builder.rs 73.68% 5 Missing ⚠️
...ates/polars-arrow/src/array/fixed_size_list/mod.rs 94.11% 1 Missing ⚠️
.../polars-arrow/src/array/fixed_size_list/mutable.rs 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #28993      +/-   ##
==========================================
+ Coverage   81.32%   81.35%   +0.03%     
==========================================
  Files        1886     1886              
  Lines      267442   267458      +16     
  Branches     3061     3061              
==========================================
+ Hits       217497   217593      +96     
+ Misses      49167    49087      -80     
  Partials      778      778              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@Angshuman09

Copy link
Copy Markdown
Author

I have implemented the changes for this issue in #28993. Would appreciate a review when you get a chance. Thanks!

@Angshuman09 Angshuman09 changed the title fix: Centralize FixedSizeList outer-to-child layout arithmetic and fix under-reservation fix(rust): Centralize FixedSizeList outer-to-child layout arithmetic and fix under-reservation Aug 28, 2026
@0guban0v

0guban0v commented Aug 31, 2026 •

Copy link
Copy Markdown
Contributor

@Angshuman09, I reviewed current diff against # 28979. Few things need to be addressed:

  • fixed_size_list/proptest.rs still uses raw length * width
  • add coverage for subslice_extend_each_repeated, gather_extend, and opt_gather_extend
  • remove Resolves #28872 from PR desc because # 28980 already resolved it

PR desc mostly repeats issue context and describes reservation fix already merged in # 28980, which obscures current scope and delay maintainer review. Cut that from desc, make scope clearer to speed up review.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

first-contribution First contribution by user fix Bug fix python Related to Python Polars rust Related to Rust Polars

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Centralize FixedSizeList outer-to-child layout conversion Fixed-size-list builders reserve outer rows instead of child values

2 participants