Skip to content

Commit 445851a

Browse files
clee704Copilot
andauthored
GH-50182: [C++][Parquet] Fix truncated min/max statistics for all-infinity floating-point columns (#50183)
### Rationale for this change When every value in a `FLOAT`, `DOUBLE`, or `FLOAT16` column is the same infinity, the Parquet writer stored a *finite* min/max statistic instead of the infinity actually present in the column: - all `+Inf` → `min` written as `FLT_MAX` / `DBL_MAX` (should be `+Inf`) - all `-Inf` → `max` written as `-FLT_MAX` / `-DBL_MAX` (should be `-Inf`) The running min/max were seeded with `std::numeric_limits<T>::max()` / `::lowest()` — the largest/smallest *finite* values. These are not identity elements for min/max once the data contains infinities: `Min(FLT_MAX, +Inf)` keeps `FLT_MAX`, so an all-`+Inf` column never displaces the seed (and symmetrically for `-Inf`). The wrong value is then written with `is_min_value_exact = true`, so a reader is told the exact minimum of an all-`+Inf` column is `FLT_MAX` — a value not even present in the column. Range-based row-group filtering stays correct (the bound is only loosened in the safe direction), but engines that read min/max statistics as values (statistics-based `MIN`/`MAX` evaluation, metadata introspection) return wrong results. See GH-50182 for a full reproduction in both PyArrow and C++. ### What changes are included in this PR? - `CompareHelper::DefaultMin()` / `DefaultMax()` now seed floating-point accumulation with `+Inf` / `-Inf` (the true identities) instead of the largest finite values; integral types are unchanged. - The same change is applied to the `Float16` comparator, via new `Float16Constants::positive_infinity()` / `negative_infinity()`. - The "no valid value was seen" detection in `CleanStatistic` / `CleanFloat16Statistic` (which relied on the old finite seeds) is updated to match the new `±Inf` seeds. An all-NaN or empty column still leaves `min = +Inf, max = -Inf` (`min > max`, which real data can never produce), so it stays unambiguously detectable and continues to emit no statistics. ### Are these changes tested? Yes. A new typed test `TestFloatStatistics.Infinities` (covering `FLOAT`, `DOUBLE`, and `FLOAT16`) asserts that all-`+Inf`, all-`-Inf`, and mixed `±Inf` columns produce the exact infinity as min/max (the existing `AssertMinMaxAre` helper also checks `is_min_value_exact`/`is_max_value_exact`). The existing all-NaN tests continue to pass, exercising the updated empty/all-NaN detection. ### Are there any user-facing changes? Parquet files written for all-infinity floating-point columns now record the correct `±Inf` min/max statistics instead of the largest finite value. No public API signatures change. Two minor behavior changes on existing public low-level APIs are worth noting (the normal writer accumulation path is unaffected, since a column with ≥1 non-NaN value always yields `min ≤ max`): - `TypedComparator<FloatType/DoubleType/Float16LogicalType>::GetMinMax()` now returns `(+Inf, -Inf)` for an all-NaN (or empty) batch, where it previously returned the largest/smallest finite value as a sentinel. - `TypedStatistics::SetMinMax()` now treats any inverted (`min > max`) input as "no valid statistics", whereas it previously only special-cased that one exact finite sentinel pair. **This PR contains a "Critical Fix".** It fixes a bug that produced incorrect data: the writer emitted min/max statistics that are wrong yet flagged exact (`is_min_value_exact = true`), which can yield incorrect results for readers that evaluate `MIN`/`MAX` directly from Parquet statistics. * GitHub Issue: #50182 Lead-authored-by: Chungmin Lee <chungminlee@microsoft.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Antoine Pitrou <antoine@python.org>
1 parent b69ba75 commit 445851a

2 files changed

Lines changed: 97 additions & 12 deletions

File tree

cpp/src/parquet/statistics.cc

Lines changed: 45 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -58,18 +58,23 @@ constexpr int value_length(int type_length, const FLBA& value) { return type_len
5858
// Static "constants" for normalizing float16 min/max values. These need to be expressed
5959
// as pointers because `Float16LogicalType` represents an FLBA.
6060
struct Float16Constants {
61-
static constexpr const uint8_t* lowest() { return lowest_.data(); }
62-
static constexpr const uint8_t* max() { return max_.data(); }
6361
static constexpr const uint8_t* positive_zero() { return positive_zero_.data(); }
6462
static constexpr const uint8_t* negative_zero() { return negative_zero_.data(); }
63+
static constexpr const uint8_t* positive_infinity() {
64+
return positive_infinity_.data();
65+
}
66+
static constexpr const uint8_t* negative_infinity() {
67+
return negative_infinity_.data();
68+
}
6569

6670
private:
6771
using Bytes = std::array<uint8_t, 2>;
68-
static constexpr Bytes lowest_ =
69-
std::numeric_limits<Float16>::lowest().ToLittleEndian();
70-
static constexpr Bytes max_ = std::numeric_limits<Float16>::max().ToLittleEndian();
7172
static constexpr Bytes positive_zero_ = (+Float16::FromBits(0)).ToLittleEndian();
7273
static constexpr Bytes negative_zero_ = (-Float16::FromBits(0)).ToLittleEndian();
74+
static constexpr Bytes positive_infinity_ =
75+
std::numeric_limits<Float16>::infinity().ToLittleEndian();
76+
static constexpr Bytes negative_infinity_ =
77+
(-std::numeric_limits<Float16>::infinity()).ToLittleEndian();
7378
};
7479

7580
template <typename DType, bool is_signed>
@@ -79,8 +84,24 @@ struct CompareHelper {
7984
static_assert(!std::is_unsigned<T>::value || std::is_same<T, bool>::value,
8085
"T is an unsigned numeric");
8186

82-
constexpr static T DefaultMin() { return std::numeric_limits<T>::max(); }
83-
constexpr static T DefaultMax() { return std::numeric_limits<T>::lowest(); }
87+
// For floating point, seed the running min/max with +/-infinity rather than
88+
// the largest/smallest finite value: a finite seed is not an identity for
89+
// min/max once the data contains infinities, so an all-+Inf (resp. all--Inf)
90+
// column would otherwise never displace the seed and report a finite bound.
91+
constexpr static T DefaultMin() {
92+
if constexpr (std::is_floating_point_v<T>) {
93+
return std::numeric_limits<T>::infinity();
94+
} else {
95+
return std::numeric_limits<T>::max();
96+
}
97+
}
98+
constexpr static T DefaultMax() {
99+
if constexpr (std::is_floating_point_v<T>) {
100+
return -std::numeric_limits<T>::infinity();
101+
} else {
102+
return std::numeric_limits<T>::lowest();
103+
}
104+
}
84105

85106
// MSVC17 fix, isnan is not overloaded for IntegralType as per C++11
86107
// standard requirements.
@@ -300,8 +321,8 @@ template <>
300321
struct CompareHelper<Float16LogicalType, /*is_signed=*/true> {
301322
using T = FLBA;
302323

303-
static T DefaultMin() { return T{Float16Constants::max()}; }
304-
static T DefaultMax() { return T{Float16Constants::lowest()}; }
324+
static T DefaultMin() { return T{Float16Constants::positive_infinity()}; }
325+
static T DefaultMax() { return T{Float16Constants::negative_infinity()}; }
305326

306327
static T Coalesce(T val, T fallback) {
307328
return (val.ptr == nullptr || Float16::FromLittleEndian(val.ptr).is_nan()) ? fallback
@@ -330,6 +351,17 @@ struct CompareHelper<Float16LogicalType, /*is_signed=*/true> {
330351

331352
using ::std::optional;
332353

354+
// A usable min/max pair always satisfies min <= max. The reverse ordering
355+
// (min > max) is produced only by the inverted DefaultMin()/DefaultMax() seeds
356+
// -- left in place when no valid, non-NaN value was observed -- or by an
357+
// inverted caller-supplied range; in either case there is no statistic to emit.
358+
// Testing the ordering keeps this independent of the specific seed values, so it
359+
// cannot drift from DefaultMin()/DefaultMax().
360+
template <typename T>
361+
bool IsInvalidMinMax(const T& min, const T& max) {
362+
return max < min;
363+
}
364+
333365
template <typename T>
334366
::arrow::enable_if_t<std::is_integral<T>::value, optional<std::pair<T, T>>>
335367
CleanStatistic(std::pair<T, T> min_max, LogicalType::Type::type) {
@@ -357,7 +389,9 @@ CleanStatistic(std::pair<T, T> min_max, LogicalType::Type::type) {
357389
return ::std::nullopt;
358390
}
359391

360-
if (min == std::numeric_limits<T>::max() && max == std::numeric_limits<T>::lowest()) {
392+
// Discard an inverted min/max: either an empty/all-NaN input left the seeds in
393+
// place, or the supplied range is invalid. A real value always has min <= max.
394+
if (IsInvalidMinMax(min, max)) {
361395
return ::std::nullopt;
362396
}
363397

@@ -384,8 +418,7 @@ optional<std::pair<FLBA, FLBA>> CleanFloat16Statistic(std::pair<FLBA, FLBA> min_
384418
return ::std::nullopt;
385419
}
386420

387-
if (min == std::numeric_limits<Float16>::max() &&
388-
max == std::numeric_limits<Float16>::lowest()) {
421+
if (IsInvalidMinMax(min, max)) {
389422
return ::std::nullopt;
390423
}
391424

cpp/src/parquet/statistics_test.cc

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1485,6 +1485,7 @@ class TestFloatStatistics : public ::testing::Test {
14851485
}
14861486

14871487
void TestNaNs();
1488+
void TestInfinities();
14881489

14891490
protected:
14901491
std::vector<uint8_t> data_buf_;
@@ -1559,6 +1560,30 @@ void TestFloatStatistics<T>::TestNaNs() {
15591560
valid_bitmap_no_nans);
15601561
}
15611562

1563+
// GH-50182: an all-infinity column must report that infinity as its min/max,
1564+
// rather than the largest finite value (a finite seed is never displaced by an
1565+
// infinity of the same sign). A NaN interspersed in such a column must be
1566+
// ignored without corrupting the infinite min/max.
1567+
template <typename T>
1568+
void TestFloatStatistics<T>::TestInfinities() {
1569+
NodePtr node = this->MakeNode("f", Repetition::REQUIRED);
1570+
ColumnDescriptor descr(node, 0, 0);
1571+
1572+
constexpr c_type inf = std::numeric_limits<c_type>::infinity();
1573+
constexpr c_type nan = std::numeric_limits<c_type>::quiet_NaN();
1574+
std::vector<c_type> all_pos_inf{inf, inf, inf};
1575+
std::vector<c_type> all_neg_inf{-inf, -inf, -inf};
1576+
std::vector<c_type> mixed_inf{inf, -inf};
1577+
std::vector<c_type> pos_inf_with_nan{inf, nan, inf};
1578+
std::vector<c_type> neg_inf_with_nan{-inf, nan, -inf};
1579+
1580+
AssertMinMaxAre(MakeStatistics<ParquetType>(&descr), all_pos_inf, inf, inf);
1581+
AssertMinMaxAre(MakeStatistics<ParquetType>(&descr), all_neg_inf, -inf, -inf);
1582+
AssertMinMaxAre(MakeStatistics<ParquetType>(&descr), mixed_inf, -inf, inf);
1583+
AssertMinMaxAre(MakeStatistics<ParquetType>(&descr), pos_inf_with_nan, inf, inf);
1584+
AssertMinMaxAre(MakeStatistics<ParquetType>(&descr), neg_inf_with_nan, -inf, -inf);
1585+
}
1586+
15621587
struct BufferedFloat16 {
15631588
explicit BufferedFloat16(Float16 f16) : f16(f16) {
15641589
this->f16.ToLittleEndian(bytes_.data());
@@ -1611,12 +1636,39 @@ void TestFloatStatistics<Float16LogicalType>::TestNaNs() {
16111636
valid_bitmap_no_nans);
16121637
}
16131638

1639+
template <>
1640+
void TestFloatStatistics<Float16LogicalType>::TestInfinities() {
1641+
NodePtr node = this->MakeNode("f", Repetition::REQUIRED);
1642+
ColumnDescriptor descr(node, 0, 0);
1643+
1644+
using F16 = BufferedFloat16;
1645+
const auto pos_inf = F16(std::numeric_limits<Float16>::infinity());
1646+
const auto neg_inf = F16(-std::numeric_limits<Float16>::infinity());
1647+
const auto nan = F16(std::numeric_limits<Float16>::quiet_NaN());
1648+
const auto pinf = FLBA{pos_inf.bytes()};
1649+
const auto ninf = FLBA{neg_inf.bytes()};
1650+
const auto fnan = FLBA{nan.bytes()};
1651+
1652+
std::vector<FLBA> all_pos_inf{pinf, pinf, pinf};
1653+
std::vector<FLBA> all_neg_inf{ninf, ninf, ninf};
1654+
std::vector<FLBA> mixed_inf{pinf, ninf};
1655+
std::vector<FLBA> pos_inf_with_nan{pinf, fnan, pinf};
1656+
std::vector<FLBA> neg_inf_with_nan{ninf, fnan, ninf};
1657+
1658+
AssertMinMaxAre(MakeStatistics<ParquetType>(&descr), all_pos_inf, pinf, pinf);
1659+
AssertMinMaxAre(MakeStatistics<ParquetType>(&descr), all_neg_inf, ninf, ninf);
1660+
AssertMinMaxAre(MakeStatistics<ParquetType>(&descr), mixed_inf, ninf, pinf);
1661+
AssertMinMaxAre(MakeStatistics<ParquetType>(&descr), pos_inf_with_nan, pinf, pinf);
1662+
AssertMinMaxAre(MakeStatistics<ParquetType>(&descr), neg_inf_with_nan, ninf, ninf);
1663+
}
1664+
16141665
using FloatingPointTypes = ::testing::Types<FloatType, DoubleType, Float16LogicalType>;
16151666

16161667
TYPED_TEST_SUITE(TestFloatStatistics, FloatingPointTypes);
16171668

16181669
TYPED_TEST(TestFloatStatistics, NegativeZeros) { this->TestNegativeZeroes(); }
16191670
TYPED_TEST(TestFloatStatistics, NaNs) { this->TestNaNs(); }
1671+
TYPED_TEST(TestFloatStatistics, Infinities) { this->TestInfinities(); }
16201672

16211673
// ARROW-7376
16221674
TEST(TestStatisticsSortOrderFloatNaN, NaNAndNullsInfiniteLoop) {

0 commit comments

Comments
 (0)