Skip to content

Conversation

@joseph-isaacs
Copy link
Contributor

No description provided.

Signed-off-by: Joe Isaacs <[email protected]>
Signed-off-by: Joe Isaacs <[email protected]>
Signed-off-by: Joe Isaacs <[email protected]>
Signed-off-by: Joe Isaacs <[email protected]>
Signed-off-by: Joe Isaacs <[email protected]>
Signed-off-by: Joe Isaacs <[email protected]>
@joseph-isaacs joseph-isaacs changed the title feat[array]: use execute to return Canonical array not Vector feat[array]: use execute to return Canonical array not Vector pt2 Jan 9, 2026
// TODO(joe): take by value
Ok(decompress_into_array(array.clone()).to_canonical())
fn execute(array: &Self::Array, ctx: &mut ExecutionCtx) -> VortexResult<Canonical> {
Ok(Canonical::Primitive(execute_decompress(
Copy link
Contributor

@0ax1 0ax1 Jan 9, 2026

Choose a reason for hiding this comment

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

Is execute_decompress a good fn name, given that execute has a particular meaning how it fits into the bigger picture? execute_decompress is just a one off inner fn. Not sure what I'm looking for here but execute_decompress suggests to me that this is a fn that is part of a pattern that is also used in other locations, which it isn't.

// We need to drop ALPArray here in case converting encoded buffer into
// primitive didn't create a copy. In that case both alp_encoded and array
// will hold a reference to the buffer we want to mutate.
drop(array);
Copy link
Contributor

Choose a reason for hiding this comment

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

no more drop?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you dont need ?

Copy link
Contributor

@0ax1 0ax1 left a comment

Choose a reason for hiding this comment

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

2 remarks

@codspeed-hq
Copy link

codspeed-hq bot commented Jan 9, 2026

Merging this PR will degrade performance by 66.33%

❌ 36 regressed benchmarks
✅ 215 untouched benchmarks
⏩ 1003 skipped benchmarks1

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

Benchmark BASE HEAD Efficiency
decompress_alp[f64, (1000, 0.0, 1.0)] 11.9 µs 18.4 µs -35.07%
decompress_alp[f64, (1000, 0.01, 1.0)] 15.1 µs 20.9 µs -27.94%
decompress_alp[f64, (1000, 0.0, 0.95)] 12.1 µs 17.9 µs -32.76%
decompress_alp[f64, (1000, 0.01, 0.25)] 15.4 µs 21.3 µs -27.57%
decompress_alp[f32, (1000, 0.0, 0.95)] 9.3 µs 13.1 µs -29.6%
decompress_alp[f64, (1000, 0.1, 1.0)] 16.3 µs 22.8 µs -28.48%
decompress_alp[f64, (1000, 0.01, 0.95)] 15.2 µs 21.1 µs -27.71%
decompress_alp[f64, (10000, 0.0, 0.25)] 54.7 µs 162 µs -66.23%
decompress_alp[f32, (1000, 0.0, 0.25)] 9.1 µs 13.1 µs -30.4%
decompress_alp[f64, (1000, 0.1, 0.25)] 15.3 µs 21.7 µs -29.47%
decompress_alp[f32, (1000, 0.01, 0.25)] 12.7 µs 16.9 µs -24.87%
decompress_alp[f32, (1000, 0.01, 0.95)] 12.5 µs 16.2 µs -22.54%
decompress_alp[f64, (1000, 0.1, 0.95)] 17.2 µs 22.5 µs -23.39%
decompress_alp[f32, (1000, 0.0, 1.0)] 9.1 µs 12.9 µs -29.6%
decompress_alp[f64, (10000, 0.0, 0.95)] 54.7 µs 162.2 µs -66.26%
decompress_alp[f32, (1000, 0.01, 1.0)] 12.4 µs 16.1 µs -23.19%
decompress_alp[f64, (10000, 0.01, 0.25)] 58.6 µs 165.9 µs -64.66%
decompress_alp[f32, (1000, 0.1, 0.25)] 12.6 µs 16.6 µs -24.36%
decompress_alp[f64, (10000, 0.01, 0.95)] 59.3 µs 166.8 µs -64.44%
decompress_alp[f64, (10000, 0.0, 1.0)] 54.6 µs 162.1 µs -66.33%
... ... ... ... ...

ℹ️ Only the first 20 benchmarks are displayed. Go to the app to view all benchmarks.


Comparing ji/execute-operator-2 (d96b06c) with develop (4b9fc90)

Open in CodSpeed

Footnotes

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

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants