Skip to content

Share fixed-width structural array operations - #8864

Draft
connortsui20 wants to merge 6 commits into
developfrom
ct/fixed-width-primitive-decimal
Draft

Share fixed-width structural array operations#8864
connortsui20 wants to merge 6 commits into
developfrom
ct/fixed-width-primitive-decimal

Conversation

@connortsui20

@connortsui20 connortsui20 commented Jul 20, 2026

Copy link
Copy Markdown
Member

Why

Primitive and Decimal are different logical types, but their structural operations both move fixed-width records from one values buffer and propagate validity. Keeping separate take/filter pipelines meant fixes and optimizations still had to be maintained twice.

What

  • add a three-hook FixedWidthArray adapter for byte width, values access, and reconstruction
  • run Primitive and Decimal through one take pipeline, including index normalization, null handling, validity propagation, piecewise ranges, and record dispatch
  • retain the direct spare-capacity copy used by the previous Primitive piecewise kernel, now over shared byte records
  • run both through one filter pipeline for values and validity
  • retain AVX2 gathers for 4- and 8-byte records; share scalar paths for 1, 2, 16, 32, and uncommon widths
  • preserve their public APIs, distinct VTables/array IDs, and serialized layouts

Review structure

  1. Share one-buffer VTable plumbing.
  2. Share the complete fixed-width take/filter machinery.

Issue provenance

  • Existing and fixed: AVX2 accepted index == len and silently zeroed some larger invalid indices.
  • Introduced during development and fixed: temporary Decimal API and arbitrary-dtype regressions, plus redundant per-range checks/capacity handling in the first shared piecewise kernel.

@codspeed-hq

codspeed-hq Bot commented Jul 20, 2026

Copy link
Copy Markdown

Merging this PR will degrade performance by 10.1%

❌ 1 regressed benchmark
✅ 1840 untouched benchmarks
⏩ 55 skipped benchmarks1

Warning

Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

Mode Benchmark BASE HEAD Efficiency
Simulation prebuilt_dense_lengths[1] 457.1 µs 508.4 µs -10.1%

Tip

Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.


Comparing ct/fixed-width-primitive-decimal (dfc6a40) with develop (e58fb58)

Open in CodSpeed

Footnotes

  1. 55 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

@connortsui20
connortsui20 force-pushed the ct/fixed-width-primitive-decimal branch 2 times, most recently from 76fd71a to f00395e Compare July 20, 2026 19:50
Comment thread vortex-array/src/array/erased.rs Outdated
Comment thread vortex-array/src/array/erased.rs Outdated
@connortsui20
connortsui20 force-pushed the ct/fixed-width-primitive-decimal branch 2 times, most recently from 35a4157 to 2e0b1e2 Compare July 20, 2026 22:05
@connortsui20
connortsui20 force-pushed the ct/fixed-width-primitive-decimal branch 2 times, most recently from 35a4157 to 2880b5f Compare July 20, 2026 22:33
@connortsui20
connortsui20 force-pushed the ct/fixed-width-primitive-decimal branch from 2880b5f to f124538 Compare July 20, 2026 22:42
connortsui20 added a commit that referenced this pull request Jul 21, 2026
## Summary

- remove the unused `ArrayRef::is_arrow` helper
- keep this reviewer-requested cleanup separate from #8864

## Testing

- `cargo check -p vortex-array`
- `cargo clippy --all-targets --all-features` (pre-push hook)

Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>
@connortsui20
connortsui20 force-pushed the ct/fixed-width-primitive-decimal branch 4 times, most recently from 6768bec to 19c65ac Compare July 22, 2026 19:29
@connortsui20 connortsui20 changed the title Unify primitive and decimal physical storage Share fixed-width structural array operations Jul 22, 2026
@connortsui20
connortsui20 force-pushed the ct/fixed-width-primitive-decimal branch 2 times, most recently from a4e7a55 to 853eb8f Compare July 23, 2026 19:15
@vortex-data vortex-data deleted a comment from github-actions Bot Jul 23, 2026
@vortex-data vortex-data deleted a comment from codecov Bot Jul 23, 2026
@vortex-data vortex-data deleted a comment from github-actions Bot Jul 23, 2026
@connortsui20
connortsui20 force-pushed the ct/fixed-width-primitive-decimal branch 3 times, most recently from 2d5d094 to 3eb9698 Compare July 27, 2026 18:25
@connortsui20
connortsui20 requested a review from robert3005 July 27, 2026 18:43
@connortsui20
connortsui20 force-pushed the ct/fixed-width-primitive-decimal branch from 3eb9698 to 74f9105 Compare July 28, 2026 15:30
@connortsui20
connortsui20 force-pushed the ct/fixed-width-primitive-decimal branch from 1bcf52b to 40e3fe6 Compare July 28, 2026 16:47
connortsui20 and others added 6 commits July 30, 2026 15:03
Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>
Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>
Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>
Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>
Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>
Address review feedback on the shared fixed-width kernels:

- Document the contract of every FixedWidthArray hook.
- Validate the records buffer length once in a shared with_values helper
  instead of in each implementation, which are now pure constructors.
- Share the byte-width dispatch between take and filter through a single
  match_each_record_width macro.
- Fix the miri gate on the AVX2 tests, which was previously a no-op
  module-level ignore attribute.
- Test the arbitrary-width fallback paths of both take and filter, and
  restore 2-byte and float filter conformance coverage.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>
@connortsui20
connortsui20 force-pushed the ct/fixed-width-primitive-decimal branch from 40e3fe6 to dfc6a40 Compare July 30, 2026 19:09
@robert3005

Copy link
Copy Markdown
Contributor

I thought there would be more lines removed, i.e. majority of Primitive and Decimal array should be gone?

@connortsui20

Copy link
Copy Markdown
Member Author

The majority is gone, the reason there's extra code is all the boilerplate and extra files

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

Labels

changelog/feature A new feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants