Skip to content

fix: emit [0] offsets for empty structural list streams (alternative to #10533) - #10534

Open
danking wants to merge 1 commit into
vortex-data:developfrom
danking:dk/fix-empty-list-zero-offset
Open

danking wants to merge 1 commit into
vortex-data:developfrom
danking:dk/fix-empty-list-zero-offset

Conversation

@danking

@danking danking commented Oct 15, 2026

Copy link
Copy Markdown
Contributor

Summary

Alternative to #10533, based on the same develop commit (18aebea), for comparing the two empty-list representations. Fixes the same ListLayoutStrategy needs at least one chunk error for empty/null nested lists.

Changes

For an empty stream, emit the offsets sentinel [0] and let the normal child writers finish the empty elements and validity streams. This preserves a structural ListLayout at each nested list level instead of returning one typed empty chunked layout for the whole empty subtree as #10533 does.

Reserve the sentinel's sequence ID before the child EOFs so it can be written without blocking behind child completion. Nonempty input continues through the existing transposition path.

Includes the same four file-roundtrip regressions and nullable/nonnullable empty-stream checks as #10533, plus persisted-layout tests at depths 1, 2, and 5 that read back each empty level's offsets and assert [0].

Validation

The [0] implementation passed the same 76 selected tests as #10533. The extended run including the new depth/layout tests is in progress. Both changed files were formatted with CI's pinned nightly-2026-09-10 rustfmt; git diff --check passes.

Implemented with OpenAI Codex assistance.

Signed-off-by: Daniel King <dan@spiraldb.com>
@danking danking self-assigned this Oct 15, 2026
@codspeed

codspeed Bot commented Oct 15, 2026

Copy link
Copy Markdown

Merging this PR will degrade performance by 3.68%

⚠️ 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.

⚠️ 25 benchmarks spent significant time in system calls

System calls cannot be consistently instrumented, so they are not included in the measure, which understates the real cost. Please switch to the Walltime instrument to accurately measure system calls.

Measurement and system calls

ℹ️ 40 benchmarks have no flame graph

The flamegraph could not be generated because we failed to resolve the symbols in the profiling trace. This happens when we fail to harvest information about language frames.

⚠️ Different runtime environments detected

Some benchmarks with significant performance changes were compared across different runtime environments,
which may affect the accuracy of the results.

Open the report in CodSpeed to investigate

⚡ 3 improved benchmarks
❌ 5 regressed benchmarks
✅ 2251 untouched benchmarks

Warning

Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

Mode Benchmark BASE HEAD Efficiency
❌ ⚠️ Simulation test_repeated_scan_scalar_at[int] 263.7 µs 355.8 µs -25.87%
❌ ⚠️ Simulation test_scan_filter_to_arrow_array[int_float] 3.5 ms 4.1 ms -14.81%
❌ ⚠️ Simulation test_repeated_scan_filter[int_float] 3.4 ms 4 ms -13.44%
❌ ⚠️ Simulation test_scan_scalar_at[int] 803.7 µs 905.7 µs -11.26%
❌ ℹ️ Simulation test_repeated_scan_filter[int] 2.5 ms 2.7 ms -10.4%
⚡ ℹ️ Simulation test_repeated_scan_scalar_at[int_float] 486.7 µs 396.8 µs +22.68%
⚡ WallTime bitpack_blocked_compress_avx512 5.6 µs 4.8 µs +18.06%
⚡ WallTime filter_neon[80] 2.9 µs 2.5 µs +17.73%

Tip

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


Comparing danking:dk/fix-empty-list-zero-offset (2c31fee) with develop (18aebea)

Open in CodSpeed

@robert3005
robert3005 requested a review from mhk197 October 16, 2026 14:05

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

I think we should close in favor of #10533. We shouldn't need to write dummy segments for correctness.

I know struct layout does this differently but not sure why @robert3005

This branch has not been deployed

No deployments
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.

2 participants