Replace ToCanonical trait usages with execute#8609
Conversation
Merging this PR will not alter performance
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ❌ | Simulation | extend_from_array_zctl[(1000, 8)] |
238.9 µs | 402 µs | -40.57% |
| ❌ | Simulation | extend_from_array_non_zctl_overlapping[(1000, 8)] |
271.3 µs | 432 µs | -37.19% |
| ❌ | Simulation | extend_from_array_non_zctl_overlapping[(1000, 32)] |
735.7 µs | 894.8 µs | -17.79% |
| ❌ | Simulation | chunked_varbinview_into_canonical[(1000, 10)] |
169.1 µs | 205.6 µs | -17.72% |
| ❌ | Simulation | slice_empty_vortex |
339.4 ns | 397.8 ns | -14.66% |
| ❌ | Simulation | extend_from_array_zctl[(1000, 64)] |
1 ms | 1.2 ms | -13.63% |
| ❌ | Simulation | extend_from_array_zctl[(10000, 8)] |
1.9 ms | 2.2 ms | -10.57% |
| ❌ | Simulation | compact_sliced[(4096, 90)] |
750 ns | 838.1 ns | -10.51% |
| ❌ | Simulation | extend_from_array_non_zctl_overlapping[(10000, 8)] |
2.2 ms | 2.5 ms | -10.41% |
| ❌ | Simulation | i32_small_overlapping |
39.9 µs | 44.3 µs | -10.02% |
| ⚡ | Simulation | chunked_bool_canonical_into[(1000, 10)] |
26.3 µs | 15.6 µs | +68.17% |
| ⚡ | Simulation | bench_many_codes_few_values[1024] |
529.7 µs | 364.2 µs | +45.47% |
| ⚡ | Simulation | chunked_varbinview_canonical_into[(100, 100)] |
259.7 µs | 224.2 µs | +15.85% |
| ⚡ | Simulation | chunked_varbinview_into_canonical[(100, 100)] |
306.7 µs | 271.1 µs | +13.15% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing claude/nice-hypatia-g68zgt (d7c69bd) with develop (88222ac)
Footnotes
-
4 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. ↩
This also removes ArrayBuilder::extend_from_array and moves that logic in to ArrayRef::append_to_builder Plumb ExecutionCtx through more methods instead of creating fresh one. Signed-off-by: Robert Kruszewski <github@robertk.io>
5e975ca to
f8f6a90
Compare
| builder: &mut dyn ArrayBuilder, | ||
| ctx: &mut ExecutionCtx, | ||
| ) -> VortexResult<()> { | ||
| let Some(builder) = builder.as_any_mut().downcast_mut::<ListBuilder<u64>>() else { |
There was a problem hiding this comment.
builder_with_capacity(DType::List, ...) returns a ListViewBuilder<u64, u64>, so generic callers that allocate the canonical builder and then append a ListArray now hit this branch and error out instead of appending. For example, zip/case_when build the default list builder and append sliced branches through append_to_builder. This should either append into the canonical ListViewBuilder or fall back through execute::<ListViewArray> before appending.
There was a problem hiding this comment.
This part needs reworking. Needs to do a different downcast here
| )? | ||
| .0; | ||
|
|
||
| let len = u.int_in_range(0..=2048)?; |
There was a problem hiding this comment.
len can now be as high as 2048, but random_strictly_sorted_ends still randomly chooses PType::U8 and later does u8::try_from(e).vortex_expect(...) after forcing the last end toward this target. Ordinary fuzzer input can therefore panic during Arbitrary instead of returning arbitrary::Error. Please choose an end type that can represent the generated max end, cap the target by the chosen type, or return an error instead of vortex_expect.
This also removes ArrayBuilder::extend_from_array and moves that logic
in to ArrayRef::append_to_builder
Plumb ExecutionCtx through more methods instead of creating fresh one.
Fixes: #3235
Signed-off-by: Robert Kruszewski github@robertk.io