From 499690b17fba5f4bb70fe6bf50cb5d0c3e11a083 Mon Sep 17 00:00:00 2001 From: jackylee-ch Date: Tue, 22 Sep 2026 09:50:35 +0800 Subject: [PATCH 1/2] fix(datafusion): report an unconvertible column statistic as absent `stats_set_to_df` ran the stored statistic and the column dtype through two `vortex_expect`s per statistic. Both sides come from the file -- the value out of the footer's stats set, the dtype off the column -- so both can legitimately fail on a file we did not write: - `Stat::dtype` returns `None` where the statistic does not apply, which is `Min`/`Max` of a null column and `Sum` of a string, list or struct column; - `Scalar::try_new` rejects a stored value that does not match the column dtype. Either one panicked inside DataFusion planning rather than returning an error or simply forgoing the statistic. Forgoing it is what the end of the same chain already did: `try_to_df().ok()` drops a scalar DataFusion cannot represent. This extends that to the whole conversion and collapses the three near-identical blocks into one helper, which is also what the `TODO(connor)` above them asked for. The statistic's `Precision` is preserved, so an exact statistic stays exact and only the unconvertible ones become `Absent`. Signed-off-by: jackylee-ch --- vortex-datafusion/src/convert/stats.rs | 127 ++++++++++++++++--------- 1 file changed, 82 insertions(+), 45 deletions(-) diff --git a/vortex-datafusion/src/convert/stats.rs b/vortex-datafusion/src/convert/stats.rs index 33a33a78ccf..6238313160f 100644 --- a/vortex-datafusion/src/convert/stats.rs +++ b/vortex-datafusion/src/convert/stats.rs @@ -2,12 +2,12 @@ // SPDX-FileCopyrightText: Copyright the Vortex contributors use datafusion_common::ColumnStatistics; +use datafusion_common::ScalarValue; use datafusion_common::stats::Precision; use vortex::array::stats::StatsSet; use vortex::dtype::DType; use vortex::dtype::Nullability; use vortex::dtype::PType; -use vortex::error::VortexExpect; use vortex::error::VortexResult; use vortex::expr::stats::Precision as VortexPrecision; use vortex::expr::stats::Stat; @@ -24,54 +24,13 @@ pub(crate) fn stats_set_to_df( // Update the total size in bytes. let column_size = stats_set.get_as::(Stat::UncompressedSizeInBytes, &PType::U64.into()); - // TODO(connor): There's a lot that can go wrong here, should probably handle this - // more gracefully... - // Find the min statistic. - let min = stats_set.get(Stat::Min).and_then(|stat_val| { - Scalar::try_new( - Stat::Min - .dtype(dtype) - .vortex_expect("must have a valid dtype"), - Some(stat_val), - ) - .vortex_expect("`Stat::Min` somehow had an incompatible `DType`") - .try_to_df() - .ok() - }); - - // Find the max statistic. - let max = stats_set.get(Stat::Max).and_then(|stat_val| { - Scalar::try_new( - Stat::Max - .dtype(dtype) - .vortex_expect("must have a valid dtype"), - Some(stat_val), - ) - .vortex_expect("`Stat::Max` somehow had an incompatible `DType`") - .try_to_df() - .ok() - }); - - // Find the sum statistic - let sum = stats_set.get(Stat::Sum).and_then(|stat_val| { - Scalar::try_new( - Stat::Sum - .dtype(dtype) - .vortex_expect("must have a valid dtype"), - Some(stat_val), - ) - .vortex_expect("`Stat::Sum` somehow had an incompatible `DType`") - .try_to_df() - .ok() - }); - let null_count = stats_set.get_as::(Stat::NullCount, &PType::U64.into()); Ok(ColumnStatistics { null_count: null_count.to_df(), - min_value: min.to_df(), - max_value: max.to_df(), - sum_value: sum.to_df(), + min_value: scalar_stat_to_df(stats_set, Stat::Min, dtype).to_df(), + max_value: scalar_stat_to_df(stats_set, Stat::Max, dtype).to_df(), + sum_value: scalar_stat_to_df(stats_set, Stat::Sum, dtype).to_df(), distinct_count: is_constant_to_distinct_count( stats_set.get_as::(Stat::IsConstant, &DType::Bool(Nullability::NonNullable)), ), @@ -79,6 +38,27 @@ pub(crate) fn stats_set_to_df( }) } +/// Read one scalar-valued statistic and convert it to DataFusion, or report it absent. +/// +/// Both halves come from the file: the stored value out of the footer's stats set, and `dtype` from +/// the column. So every step can legitimately fail on a file we did not write — `Stat::dtype` +/// returns `None` where the statistic does not apply (`Min`/`Max` of a null column, `Sum` of a +/// string, list or struct column), and `Scalar::try_new` rejects a stored value that does not match +/// the column dtype. A statistic we cannot convert becomes `Absent`, which is what the DataFusion +/// conversion at the end of the chain already did with the values it could not represent. +fn scalar_stat_to_df( + stats_set: &StatsSet, + stat: Stat, + dtype: &DType, +) -> VortexPrecision { + stats_set.get(stat).and_then(|stat_value| { + Scalar::try_new(stat.dtype(dtype)?, Some(stat_value)) + .ok()? + .try_to_df() + .ok() + }) +} + pub(crate) fn is_constant_to_distinct_count( is_constant: VortexPrecision, ) -> Precision { @@ -90,7 +70,10 @@ pub(crate) fn is_constant_to_distinct_count( #[cfg(test)] mod tests { + use datafusion_common::ScalarValue as DFScalarValue; + use rstest::rstest; use vortex::expr::stats::Precision as VortexPrecision; + use vortex::scalar::ScalarValue; use super::*; @@ -108,4 +91,58 @@ mod tests { Ok(()) } + + /// A statistic the column dtype cannot carry has to come back `Absent`. Both sides of this + /// conversion are read out of the file, so a footer can pair a statistic with a dtype that has + /// no scalar type for it, or with a value of the wrong type. + #[rstest] + // `Stat::Min`/`Max` have no dtype for a null column. + #[case::min_of_null_column(Stat::Min, DType::Null, ScalarValue::from(1i32))] + #[case::max_of_null_column(Stat::Max, DType::Null, ScalarValue::from(1i32))] + // `Sum` has no dtype for a string column. + #[case::sum_of_utf8_column( + Stat::Sum, + DType::Utf8(Nullability::NonNullable), + ScalarValue::from(1i32) + )] + // A stored value the column dtype cannot hold. + #[case::value_disagrees_with_dtype( + Stat::Min, + DType::Bool(Nullability::NonNullable), + ScalarValue::from("not a bool") + )] + fn unconvertible_statistics_are_absent( + #[case] stat: Stat, + #[case] dtype: DType, + #[case] value: ScalarValue, + ) -> VortexResult<()> { + let stats = stats_set_to_df(&StatsSet::of(stat, VortexPrecision::exact(value)), &dtype)?; + + for reported in [stats.min_value, stats.max_value, stats.sum_value] { + assert_eq!(reported, Precision::Absent); + } + Ok(()) + } + + /// The precision of a statistic that does convert must survive, rather than everything + /// collapsing to `Absent` or to `Exact`. + #[rstest] + #[case::exact( + VortexPrecision::exact(ScalarValue::from(7i32)), + Precision::Exact(DFScalarValue::Int32(Some(7))) + )] + #[case::inexact( + VortexPrecision::inexact(ScalarValue::from(7i32)), + Precision::Inexact(DFScalarValue::Int32(Some(7))) + )] + fn convertible_statistics_keep_their_precision( + #[case] min: VortexPrecision, + #[case] expected: Precision, + ) -> VortexResult<()> { + let dtype = DType::Primitive(PType::I32, Nullability::NonNullable); + let stats = stats_set_to_df(&StatsSet::of(Stat::Min, min), &dtype)?; + + assert_eq!(stats.min_value, expected); + Ok(()) + } } From dc840ae3653f7480f589f788dfe6d5554509da4f Mon Sep 17 00:00:00 2001 From: jackylee-ch Date: Tue, 22 Sep 2026 17:26:25 +0800 Subject: [PATCH 2/2] review: trim the "why" from the stats conversion comments Per myrrc's review. The rationale now lives in the PR description, not the code. Signed-off-by: jackylee-ch --- vortex-datafusion/src/convert/stats.rs | 20 ++++---------------- 1 file changed, 4 insertions(+), 16 deletions(-) diff --git a/vortex-datafusion/src/convert/stats.rs b/vortex-datafusion/src/convert/stats.rs index 6238313160f..44f43ab4383 100644 --- a/vortex-datafusion/src/convert/stats.rs +++ b/vortex-datafusion/src/convert/stats.rs @@ -38,14 +38,8 @@ pub(crate) fn stats_set_to_df( }) } -/// Read one scalar-valued statistic and convert it to DataFusion, or report it absent. -/// -/// Both halves come from the file: the stored value out of the footer's stats set, and `dtype` from -/// the column. So every step can legitimately fail on a file we did not write — `Stat::dtype` -/// returns `None` where the statistic does not apply (`Min`/`Max` of a null column, `Sum` of a -/// string, list or struct column), and `Scalar::try_new` rejects a stored value that does not match -/// the column dtype. A statistic we cannot convert becomes `Absent`, which is what the DataFusion -/// conversion at the end of the chain already did with the values it could not represent. +/// Read one scalar-valued statistic and convert it to DataFusion, or `Absent` if it does not apply +/// to `dtype` or cannot be represented. fn scalar_stat_to_df( stats_set: &StatsSet, stat: Stat, @@ -92,20 +86,15 @@ mod tests { Ok(()) } - /// A statistic the column dtype cannot carry has to come back `Absent`. Both sides of this - /// conversion are read out of the file, so a footer can pair a statistic with a dtype that has - /// no scalar type for it, or with a value of the wrong type. + /// A statistic the column dtype cannot carry comes back `Absent`. #[rstest] - // `Stat::Min`/`Max` have no dtype for a null column. #[case::min_of_null_column(Stat::Min, DType::Null, ScalarValue::from(1i32))] #[case::max_of_null_column(Stat::Max, DType::Null, ScalarValue::from(1i32))] - // `Sum` has no dtype for a string column. #[case::sum_of_utf8_column( Stat::Sum, DType::Utf8(Nullability::NonNullable), ScalarValue::from(1i32) )] - // A stored value the column dtype cannot hold. #[case::value_disagrees_with_dtype( Stat::Min, DType::Bool(Nullability::NonNullable), @@ -124,8 +113,7 @@ mod tests { Ok(()) } - /// The precision of a statistic that does convert must survive, rather than everything - /// collapsing to `Absent` or to `Exact`. + /// A statistic that does convert keeps its precision. #[rstest] #[case::exact( VortexPrecision::exact(ScalarValue::from(7i32)),