Skip to content

Feat/sequence cast compute#8403

Open
siddarth2810 wants to merge 2 commits into
vortex-data:developfrom
siddarth2810:feat/sequence-cast-compute
Open

Feat/sequence cast compute#8403
siddarth2810 wants to merge 2 commits into
vortex-data:developfrom
siddarth2810:feat/sequence-cast-compute

Conversation

@siddarth2810

Copy link
Copy Markdown

Summary

This PR updates SequenceArray to track the arithmetic ptype separately from the declared output dtype.

Previously, SequenceArray used the same ptype for both calculation and output. Cases like a negative-step signed sequence where all generated values still fit an unsigned output dtype failed.

This change stores calculation_ptype in the SequenceMetadata, validates generated values against the output dtype, and updates sequence paths to compute using calculation_ptype while emitting values using the array output ptype.

After casting, a SequenceArray can compute internally as i32 while exposing u8 as its array dtype. Added a test that catches scalar_at returning an i32 PValue inside a u8 scalar.

Closes: #5102

API Changes

Sequence::try_new and Sequence::new_unchecked now accept both calculation_ptype and output_ptype.

Testing

For testing code changes

cargo nextest run -p vortex-sequence
cargo +nightly fmt --all
cargo clippy --all-targets --all-features

For testing the build

cargo build --workspace

AI tools Disclosure

Used ChatGPT for understanding code and OpenCode for updating callers and generating tests

SequenceArray previously used the same ptype for arithmetic and for the declared
output dtype, which made the model too narrow for casts.

Store calculation_ptype separately from the output dtype, preserve it through
metadata, and validate that generated values fit the declared output type.

Update decompression, filter, take, slice, scalar access, and min/max paths
to compute in calculation_ptype while emitting values using the array output
ptype.

Signed-off-by: Siddarth Gundu <siddarthg0910@gmail.com>
Sequence::try_new now accepts both the calculation ptype and output ptype.

Signed-off-by: Siddarth Gundu <siddarthg0910@gmail.com>
@siddarth2810 siddarth2810 requested a review from a team June 13, 2026 18:06
@connortsui20 connortsui20 self-requested a review June 13, 2026 23:47
@connortsui20 connortsui20 added the changelog/fix A bug fix label Jun 13, 2026
@connortsui20

Copy link
Copy Markdown
Member

Don't worry about the rustsec issue, that is a known problem

@codspeed-hq

codspeed-hq Bot commented Jun 13, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

⚠️ Unknown Walltime execution environment detected

Using the Walltime instrument on standard Hosted Runners will lead to inconsistent data.

For the most accurate results, we recommend using CodSpeed Macro Runners: bare-metal machines fine-tuned for performance measurement consistency.

⚡ 4 improved benchmarks
❌ 2 regressed benchmarks
✅ 1539 untouched benchmarks
⏩ 10 skipped benchmarks1

Warning

Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

Mode Benchmark BASE HEAD Efficiency
Simulation decompress_rd[f64, (100000, 0.01)] 845.9 µs 981.6 µs -13.83%
Simulation decompress_rd[f64, (100000, 0.1)] 845.9 µs 981.6 µs -13.82%
Simulation decompress_rd[f64, (100000, 0.0)] 1,024.6 µs 845.9 µs +21.12%
Simulation decompress_rd[f32, (100000, 0.0)] 586.8 µs 499.3 µs +17.51%
Simulation bitwise_not_vortex_buffer_mut[128] 244.4 ns 215.3 ns +13.55%
Simulation bitwise_not_vortex_buffer_mut[1024] 304.7 ns 275.6 ns +10.58%

Tip

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


Comparing siddarth2810:feat/sequence-cast-compute (c73544f) with develop (9444d20)

Open in CodSpeed

Footnotes

  1. 10 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.

@gatesn gatesn left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a good catch that this was broken before.

But why is it important to store this information? Vs just always computing in i64/u64 space?

@siddarth2810

Copy link
Copy Markdown
Author

It's a good catch that this was broken before.

But why is it important to store this information? Vs just always computing in i64/u64 space?

Thanks for the quick review :)

The maintainer in the previous PR mentioned to use two ptypes, so I went ahead with that design in mind.

But after experimenting a bit on this, I think we could get calculation_ptype from base.ptype()
instead of being passed around or stored separately.

For deserialization, I think we can use scalar kind before decoding the scalar, so we may not need to store calculation_ptype in SequenceMetadata either.

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

Labels

changelog/fix A bug fix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

SequenceArray cannot cast to a narrower type

3 participants