Skip to content

Keep the file sum absent when a chunk sum overflows - #10328

Merged
connortsui20 merged 3 commits into
developfrom
ct/determined-noether-n06jvq
Oct 6, 2026
Merged

connortsui20 merged 3 commits into
developfrom
ct/determined-noether-n06jvq

Conversation

@connortsui20

@connortsui20 connortsui20 commented Oct 6, 2026 •

Copy link
Copy Markdown
Member

Summary

The file writer computes the file Stat::Sum in two passes. push_chunk stores each chunk's sum in a nullable column, and as_stats_set sums that column again. Both passes use the legacy Sum aggregate, which returns zero for empty input and null only on overflow. The second pass skips nulls as if they were missing values, so the footer could report an exact sum of only the chunks that did not overflow. For chunks [i64::MAX, 1] and [2], the footer reported 2.

This happens with real data when some chunks overflow and others don't. Examples: microsecond timestamps stored as plain i64, where full batches overflow but a short last batch does not, or i64::MAX / u64::MAX sentinel values that only appear in some chunks.

This replaces #10228 with a much smaller fix.

Changes

  • In StatsAccumulator::as_stats_set, leave Sum absent when any chunk sum is null. This follows the existing guard for truncated varlen Max right above it.
  • Adds an rstest unit test in vortex-layout (one chunk overflows, all chunks overflow, the total overflows, nullable values, an all-null chunk) and an end-to-end writer test in vortex-file that checks both the returned footer and the footer read back from the file.

This is a stopgap for the legacy Stat::Sum path only. When file statistics move to aggregate function partials (for example SumV2, which tracks overflow in an explicit is_overflow field), chunk results are merged as partials instead of summed as values, and this guard can be deleted along with Stat::Sum.

🤖 Generated with Claude Code

https://claude.ai/code/session_014jTJihcDfBHCS9AQcQHLjR

The file writer sums a nullable column of per-chunk sums. A chunk sum is
null only when that chunk overflowed, and the outer sum skipped those
nulls, so the footer could report an exact sum of just the chunks that did
not overflow. For chunks `[i64::MAX, 1]` and `[2]` the footer reported 2.

Leave the file sum absent when any chunk sum is null, matching the
existing guard for truncated varlen maxima.

Claude-Session: https://claude.ai/code/session_014jTJihcDfBHCS9AQcQHLjR
Signed-off-by: Claude <noreply@anthropic.com>
@connortsui20
connortsui20 marked this pull request as ready for review October 6, 2026 11:14
@connortsui20 connortsui20 added the changelog/fix A bug fix label Oct 6, 2026
Comment thread vortex-layout/src/layouts/file_stats.rs Outdated
@connortsui20
connortsui20 enabled auto-merge (squash) October 6, 2026 11:36
Claude-Session: https://claude.ai/code/session_014jTJihcDfBHCS9AQcQHLjR
Signed-off-by: Claude <noreply@anthropic.com>
@codspeed

codspeed Bot commented Oct 6, 2026

Copy link
Copy Markdown

Merging this PR will degrade performance by 25.65%

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

⚠️ 2 benchmarks measured no execution time

Nothing ran under measurement, usually because the compiler removed the code under test. These results are not comparable, so they count as unchanged.

Preventing compiler optimizations

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

❌ 1 regressed benchmark
✅ 2100 untouched benchmarks
⏩ 518 skipped benchmarks1

Warning

Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

Mode Benchmark BASE HEAD Efficiency
❌ Simulation new_bp_prim_test_between[i64, 2048] 96.9 µs 130.3 µs -25.65%
⚠️ Simulation density_sweep_dense_runs[0.9] 52.8 µs < 1 ns N/A
⚠️ Simulation bench_compare_sliced_dict_primitive[(3333, 10000)] 77.9 µs < 1 ns N/A

Tip

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


Comparing ct/determined-noether-n06jvq (eccd65d) with develop (5cfc0b5)

Open in CodSpeed

Footnotes

  1. 518 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. ↩

@connortsui20
connortsui20 merged commit 3c87da0 into develop Oct 6, 2026
89 of 90 checks passed
@connortsui20
connortsui20 deleted the ct/determined-noether-n06jvq branch October 6, 2026 13:19
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.

3 participants