From 4f8145e319402b8cfc7708460643df6c14a089b2 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 6 Oct 2026 11:13:03 +0000 Subject: [PATCH 1/3] Keep the file sum absent when a chunk sum overflows 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 --- vortex-file/src/tests.rs | 34 +++++++++++++++++++++++++ vortex-layout/src/layouts/file_stats.rs | 32 +++++++++++++++++++++++ 2 files changed, 66 insertions(+) diff --git a/vortex-file/src/tests.rs b/vortex-file/src/tests.rs index 6f74e583b7c..3b0f6ddc49b 100644 --- a/vortex-file/src/tests.rs +++ b/vortex-file/src/tests.rs @@ -54,6 +54,7 @@ use vortex_array::expr::lt_eq; use vortex_array::expr::or; use vortex_array::expr::root; use vortex_array::expr::select; +use vortex_array::expr::stats::Stat; use vortex_array::extension::datetime::TimeUnit; use vortex_array::extension::datetime::Timestamp; use vortex_array::extension::datetime::TimestampOptions; @@ -2126,6 +2127,39 @@ async fn test_writer_with_statistics() -> VortexResult<()> { Ok(()) } +#[tokio::test] +async fn file_sum_is_absent_when_a_chunk_overflows() -> VortexResult<()> { + let dtype = DType::Struct( + StructFields::from_iter([("numbers", DType::from(PType::I64))]), + Nullability::NonNullable, + ); + let mut buf = ByteBufferMut::empty(); + let mut writer = SESSION + .write_options() + .with_file_statistics(vec![Stat::Sum]) + .writer(&mut buf, dtype); + + // The first chunk overflows, so the file sum must not be the second chunk's sum of 2. + for chunk in [buffer![i64::MAX, 1], buffer![2i64]] { + writer + .push(StructArray::from_fields(&[("numbers", chunk.into_array())])?.into_array()) + .await?; + } + + let summary = writer.finish().await?; + let footer_stats = summary + .footer() + .statistics() + .vortex_expect("file statistics were requested"); + assert!(footer_stats.stats_sets()[0].get(Stat::Sum).is_absent()); + + let file = SESSION.open_options().open_buffer(buf)?; + let file_stats = file.file_stats().vortex_expect("file statistics were written"); + assert!(file_stats.stats_sets()[0].get(Stat::Sum).is_absent()); + + Ok(()) +} + #[tokio::test] async fn test_file_metadata_roundtrip() -> VortexResult<()> { let array = diff --git a/vortex-layout/src/layouts/file_stats.rs b/vortex-layout/src/layouts/file_stats.rs index 332012c8cd4..6aefc380866 100644 --- a/vortex-layout/src/layouts/file_stats.rs +++ b/vortex-layout/src/layouts/file_stats.rs @@ -158,6 +158,11 @@ impl StatsAccumulator { // upper bound, so aggregating by skipping nulls would be unsound. continue; } + Stat::Sum if !values.all_valid(ctx)? => { + // A chunk sum is only null when that chunk overflowed, so the file sum is + // unknown. Aggregating by skipping nulls would return a wrong exact sum. + continue; + } Stat::Min | Stat::Max | Stat::Sum => { if let Some(s) = values.statistics().compute_stat(stat, ctx)? && let Some(v) = s.into_value() @@ -531,6 +536,7 @@ mod tests { use vortex_array::IntoArray; use vortex_array::array_session; use vortex_array::arrays::BoolArray; + use vortex_array::arrays::PrimitiveArray; use vortex_array::arrays::bool::BoolArrayExt; use vortex_array::builders::VarBinViewBuilder; use vortex_buffer::BitBuffer; @@ -636,4 +642,30 @@ mod tests { &[Stat::Max.name(), Stat::Min.name(), Stat::Sum.name()] ); } + + #[rstest] + #[case::one_chunk_overflows(vec![vec![Some(i64::MAX), Some(1)], vec![Some(2)]], None)] + #[case::all_chunks_overflow(vec![vec![Some(i64::MAX), Some(1)]; 2], None)] + #[case::total_overflows(vec![vec![Some(i64::MAX)], vec![Some(1)]], None)] + #[case::nullable_values(vec![vec![None, Some(3)], vec![Some(4), None]], Some(7))] + #[case::all_null_chunk(vec![vec![None], vec![Some(5)]], Some(5))] + fn combines_chunk_sums( + #[case] chunks: Vec>>, + #[case] expected: Option, + ) -> VortexResult<()> { + let mut ctx = array_session().create_execution_ctx(); + let dtype = DType::Primitive(PType::I64, Nullability::Nullable); + let mut acc = StatsAccumulator::new(&dtype, &[Stat::Sum], 12); + for chunk in chunks { + acc.push_chunk(&PrimitiveArray::from_option_iter(chunk).into_array(), &mut ctx)?; + } + + let stats = acc.as_stats_set(&[Stat::Sum], &mut ctx)?; + assert_eq!( + stats.get(Stat::Sum), + expected.map_or(Precision::Absent, Precision::exact) + ); + + Ok(()) + } } From 7b91efa6c468154eea15fff6029a66d5ab364533 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 6 Oct 2026 11:34:31 +0000 Subject: [PATCH 2/3] Clarify which sum the file sum overflow guard refers to Claude-Session: https://claude.ai/code/session_014jTJihcDfBHCS9AQcQHLjR Signed-off-by: Claude --- vortex-layout/src/layouts/file_stats.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/vortex-layout/src/layouts/file_stats.rs b/vortex-layout/src/layouts/file_stats.rs index 6aefc380866..02f39b35af3 100644 --- a/vortex-layout/src/layouts/file_stats.rs +++ b/vortex-layout/src/layouts/file_stats.rs @@ -159,8 +159,10 @@ impl StatsAccumulator { continue; } Stat::Sum if !values.all_valid(ctx)? => { - // A chunk sum is only null when that chunk overflowed, so the file sum is - // unknown. Aggregating by skipping nulls would return a wrong exact sum. + // `values` holds each chunk's `Stat::Sum` from `push_chunk`. The legacy `Sum` + // aggregate behind it returns zero for empty input and null only on overflow. + // Summing `values` below would skip those nulls and report a wrong exact + // total, so leave the file sum unset. continue; } Stat::Min | Stat::Max | Stat::Sum => { From eccd65d34a20fc150a90c258f6b4724b739c6f70 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 6 Oct 2026 13:05:07 +0000 Subject: [PATCH 3/3] Format file sum overflow tests Claude-Session: https://claude.ai/code/session_014jTJihcDfBHCS9AQcQHLjR Signed-off-by: Claude --- vortex-file/src/tests.rs | 4 +++- vortex-layout/src/layouts/file_stats.rs | 5 ++++- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/vortex-file/src/tests.rs b/vortex-file/src/tests.rs index 3b0f6ddc49b..083d6bcdf6e 100644 --- a/vortex-file/src/tests.rs +++ b/vortex-file/src/tests.rs @@ -2154,7 +2154,9 @@ async fn file_sum_is_absent_when_a_chunk_overflows() -> VortexResult<()> { assert!(footer_stats.stats_sets()[0].get(Stat::Sum).is_absent()); let file = SESSION.open_options().open_buffer(buf)?; - let file_stats = file.file_stats().vortex_expect("file statistics were written"); + let file_stats = file + .file_stats() + .vortex_expect("file statistics were written"); assert!(file_stats.stats_sets()[0].get(Stat::Sum).is_absent()); Ok(()) diff --git a/vortex-layout/src/layouts/file_stats.rs b/vortex-layout/src/layouts/file_stats.rs index 02f39b35af3..3d031a65c7a 100644 --- a/vortex-layout/src/layouts/file_stats.rs +++ b/vortex-layout/src/layouts/file_stats.rs @@ -659,7 +659,10 @@ mod tests { let dtype = DType::Primitive(PType::I64, Nullability::Nullable); let mut acc = StatsAccumulator::new(&dtype, &[Stat::Sum], 12); for chunk in chunks { - acc.push_chunk(&PrimitiveArray::from_option_iter(chunk).into_array(), &mut ctx)?; + acc.push_chunk( + &PrimitiveArray::from_option_iter(chunk).into_array(), + &mut ctx, + )?; } let stats = acc.as_stats_set(&[Stat::Sum], &mut ctx)?;