Skip to content

fix(layout): rebase sliced list chunks before writing the list layout - #11082

Merged
mhk197 merged 1 commit into
vortex-data:developfrom
gitRasheed:list-writer-sliced-chunks
Nov 3, 2026
Merged

mhk197 merged 1 commit into
vortex-data:developfrom
gitRasheed:list-writer-sliced-chunks

Conversation

@gitRasheed

@gitRasheed gitRasheed commented Nov 2, 2026 •

Copy link
Copy Markdown
Contributor

Summary

ListLayoutStrategy assumes each ListArray chunk's offsets start at 0 and end at elements.len(). A sliced ListArray keeps its parent's whole elements, so the writer stores offsets that point at the wrong elements, and the data reads back wrong with no error. For example, the lists [0, 1, 2], [], [3, 4, 5], [6, 7], [8, 9] written as three slices read back with row 1 as [3, 4, 5, 6, 7, 8, 9, 0, 1, 2] instead of [].

The default file writer is not affected. It repartitions each list column into a canonical ListView first, and that conversion trims the elements. The bug hits writers that pass ListArray chunks to the list strategy directly, such as TableStrategy::with_list_layout().

Changes

  • Call reset_offsets on each ListArray chunk before writing it.

Tests

sliced_list_chunks_round_trip writes slices of one list through the list writer and reads them back, with a null list and an empty list at the cuts. sliced_nested_list_chunks_round_trip does the same for list<list<i32>>, where the inner lists arrive sliced too. Both fail without the change.

Checked locally: the vortex-layout list tests, clippy --all-targets and fmt --check for vortex-layout and vortex-file.

I used AI tools to help write and review this change.

@robert3005
robert3005 requested a review from mhk197 November 2, 2026 13:42

@mhk197 mhk197 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.

Thanks for your contribution! I requested some changes wrt unnecessary tests, but the overall change is good.

Comment thread vortex-file/src/tests.rs Resolved
The list writer stores each chunk's whole elements and shifts its offsets
by the elements written before it, which assumes the offsets span exactly
[0, elements.len()]. A sliced ListArray keeps its parent's elements, so
its lists read back as the wrong elements. Reset each ListArray chunk's
offsets first. ListView chunks, and so the default file writer, which
canonicalizes before the list writer, were already trimmed.

Signed-off-by: gitRasheed <[email protected]>
@codspeed

codspeed Bot commented Nov 3, 2026

Copy link
Copy Markdown

Merging this PR will degrade performance by 11.35%

2 warnings

⚠️ Walltime results may be unreliable

Some benchmarks used the Walltime instrument outside CodSpeed Macro Runners, which can make the results inconsistent.

Learn more about Macro Runners

See affected benchmarks

⚠️ 1 benchmark is dominated by system calls

System calls cannot be simulated consistently and are left out of the measure. The reported value understates the real cost. Switch to the Walltime instrument to measure them.

Learn more about system calls

See affected benchmarks

❌ 1 regressed benchmark
✅ 2317 untouched benchmarks

Warning

Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

Mode Benchmark BASE HEAD Efficiency
❌ Walltime starts_with_avx512[64, VarBin] 53.5 µs 60.4 µs -11.35%

Tip

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


Comparing gitRasheed:list-writer-sliced-chunks (d842053) with develop (d8e7467)

Open in CodSpeed

@mhk197 mhk197 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.

lgtm now

@mhk197
mhk197 merged commit 585a1cd into vortex-data:develop Nov 3, 2026
106 of 108 checks passed
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.

2 participants