Skip to content

Commit 3c87da0

Browse files
connortsui20claude
andauthored
Keep the file sum absent when a chunk sum overflows (#10328)
## Summary - Tracking Issue: #10177 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.com/claude-code) https://claude.ai/code/session_014jTJihcDfBHCS9AQcQHLjR --------- Signed-off-by: Claude <noreply@anthropic.com> Co-authored-by: Claude <noreply@anthropic.com>
1 parent 90c40ee commit 3c87da0

2 files changed

Lines changed: 73 additions & 0 deletions

File tree

‎vortex-file/src/tests.rs‎

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,7 @@ use vortex_array::expr::lt_eq;
5454
use vortex_array::expr::or;
5555
use vortex_array::expr::root;
5656
use vortex_array::expr::select;
57+
use vortex_array::expr::stats::Stat;
5758
use vortex_array::extension::datetime::TimeUnit;
5859
use vortex_array::extension::datetime::Timestamp;
5960
use vortex_array::extension::datetime::TimestampOptions;
@@ -2126,6 +2127,41 @@ async fn test_writer_with_statistics() -> VortexResult<()> {
21262127
Ok(())
21272128
}
21282129

2130+
#[tokio::test]
2131+
async fn file_sum_is_absent_when_a_chunk_overflows() -> VortexResult<()> {
2132+
let dtype = DType::Struct(
2133+
StructFields::from_iter([("numbers", DType::from(PType::I64))]),
2134+
Nullability::NonNullable,
2135+
);
2136+
let mut buf = ByteBufferMut::empty();
2137+
let mut writer = SESSION
2138+
.write_options()
2139+
.with_file_statistics(vec![Stat::Sum])
2140+
.writer(&mut buf, dtype);
2141+
2142+
// The first chunk overflows, so the file sum must not be the second chunk's sum of 2.
2143+
for chunk in [buffer![i64::MAX, 1], buffer![2i64]] {
2144+
writer
2145+
.push(StructArray::from_fields(&[("numbers", chunk.into_array())])?.into_array())
2146+
.await?;
2147+
}
2148+
2149+
let summary = writer.finish().await?;
2150+
let footer_stats = summary
2151+
.footer()
2152+
.statistics()
2153+
.vortex_expect("file statistics were requested");
2154+
assert!(footer_stats.stats_sets()[0].get(Stat::Sum).is_absent());
2155+
2156+
let file = SESSION.open_options().open_buffer(buf)?;
2157+
let file_stats = file
2158+
.file_stats()
2159+
.vortex_expect("file statistics were written");
2160+
assert!(file_stats.stats_sets()[0].get(Stat::Sum).is_absent());
2161+
2162+
Ok(())
2163+
}
2164+
21292165
#[tokio::test]
21302166
async fn test_file_metadata_roundtrip() -> VortexResult<()> {
21312167
let array =

‎vortex-layout/src/layouts/file_stats.rs‎

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -158,6 +158,13 @@ impl StatsAccumulator {
158158
// upper bound, so aggregating by skipping nulls would be unsound.
159159
continue;
160160
}
161+
Stat::Sum if !values.all_valid(ctx)? => {
162+
// `values` holds each chunk's `Stat::Sum` from `push_chunk`. The legacy `Sum`
163+
// aggregate behind it returns zero for empty input and null only on overflow.
164+
// Summing `values` below would skip those nulls and report a wrong exact
165+
// total, so leave the file sum unset.
166+
continue;
167+
}
161168
Stat::Min | Stat::Max | Stat::Sum => {
162169
if let Some(s) = values.statistics().compute_stat(stat, ctx)?
163170
&& let Some(v) = s.into_value()
@@ -531,6 +538,7 @@ mod tests {
531538
use vortex_array::IntoArray;
532539
use vortex_array::array_session;
533540
use vortex_array::arrays::BoolArray;
541+
use vortex_array::arrays::PrimitiveArray;
534542
use vortex_array::arrays::bool::BoolArrayExt;
535543
use vortex_array::builders::VarBinViewBuilder;
536544
use vortex_buffer::BitBuffer;
@@ -636,4 +644,33 @@ mod tests {
636644
&[Stat::Max.name(), Stat::Min.name(), Stat::Sum.name()]
637645
);
638646
}
647+
648+
#[rstest]
649+
#[case::one_chunk_overflows(vec![vec![Some(i64::MAX), Some(1)], vec![Some(2)]], None)]
650+
#[case::all_chunks_overflow(vec![vec![Some(i64::MAX), Some(1)]; 2], None)]
651+
#[case::total_overflows(vec![vec![Some(i64::MAX)], vec![Some(1)]], None)]
652+
#[case::nullable_values(vec![vec![None, Some(3)], vec![Some(4), None]], Some(7))]
653+
#[case::all_null_chunk(vec![vec![None], vec![Some(5)]], Some(5))]
654+
fn combines_chunk_sums(
655+
#[case] chunks: Vec<Vec<Option<i64>>>,
656+
#[case] expected: Option<i64>,
657+
) -> VortexResult<()> {
658+
let mut ctx = array_session().create_execution_ctx();
659+
let dtype = DType::Primitive(PType::I64, Nullability::Nullable);
660+
let mut acc = StatsAccumulator::new(&dtype, &[Stat::Sum], 12);
661+
for chunk in chunks {
662+
acc.push_chunk(
663+
&PrimitiveArray::from_option_iter(chunk).into_array(),
664+
&mut ctx,
665+
)?;
666+
}
667+
668+
let stats = acc.as_stats_set(&[Stat::Sum], &mut ctx)?;
669+
assert_eq!(
670+
stats.get(Stat::Sum),
671+
expected.map_or(Precision::Absent, Precision::exact)
672+
);
673+
674+
Ok(())
675+
}
639676
}

0 commit comments

Comments
 (0)