This is an automated email from the ASF dual-hosted git repository.
yiguolei pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/doris.git
The following commit(s) were added to refs/heads/master by this push:
new 1a141d30e91 [fix](zonemap) Do not treat a legacy float or double zone
map as NaN-free (#67779)
1a141d30e91 is described below
commit 1a141d30e9104cf18895263b52c44c2fb43cd92c
Author: YangJie <[email protected]>
AuthorDate: Wed Sep 16 22:28:08 2026 -0400
[fix](zonemap) Do not treat a legacy float or double zone map as NaN-free
(#67779)
### What problem does this PR solve?
Issue Number: close #67773
Problem Summary:
`ZoneMapPB` fields 6 to 8 (`has_positive_inf`, `has_negative_inf`,
`has_nan`) were all added in the same change that made float and double
zone maps NaN-aware. `ZoneMap::from_proto` reads `zone_map.has_nan()`
without consulting `has_has_nan()`, so a zone map serialized before
those fields existed and one written by the current code for a genuinely
NaN-free zone deserialize to the same in-memory state.
That matters because the two are not the same. Float and double zone
maps already existed before the flags did, and their bounds were
computed by a comparison that never selects a NaN: a page holding `{1.0,
NaN, 2.0}` stores `[1.0, 2.0]` and has nowhere to record the NaN. Doris
orders NaN above every other value, so `WHERE d > 3.0` prunes that page
even though the NaN row satisfies the predicate. The query loses a row
and reports no error.
The path is reachable on upgrade rather than only in theory. The 3.1
line has no `has_nan` field, the 4.x readers have the unguarded read,
and an upgrade replaces binaries while leaving existing data files in
place, so a 4.x BE can read pre-4.0 rowsets immediately. The
reversed-bounds guard added in #67431 does not cover this: it catches
legacy pages whose bounds never moved, such as NaN-only pages, while a
mixed finite and NaN page has ordinary ordered bounds.
`from_proto` now treats an absent field 8 on a float or double column as
"the NaN state is unknown" and marks the zone map `pass_all`, which is
the same degradation the function already applies to a bound that fails
to parse and to reversed bounds. Field presence is a better signal than
a segment version gate: the writer calls `set_has_nan(false)` explicitly
for a NaN-free zone, so presence separates current known-false metadata
from legacy unknown metadata. Keeping this in the deserialization layer
means segment pruning, page pruning, predicate elimination and
expression zone map evaluation all inherit the conservative behaviour; a
check in one consumer would leave the others exposed. `has_null` and
`has_not_null` are untouched. What that buys depends on the path:
`eval_null_zonemap` reads only those two flags and never consults
`pass_all`, so IS NULL and IS NOT NULL keep pruning on the expression
zone map path, while the ColumnPredicate path in
`ColumnReader::_get_filtered_pages` short-circuits on `pass_all` and
loses its IS NULL pruning for these zone maps. Pushed-down min/max
aggregation also stops for them, because
`segment_zone_maps_can_answer_agg` marks a `pass_all` segment unusable,
so `SELECT max(d)` falls back to a scan.
The guard only fires for a zone map that has a non-null value. An
all-null zone never received one, so no NaN can hide in it, and leaving
it usable keeps the three ColumnPredicate checks that open with
`has_not_null`: the null predicates, each comparison predicate's early
return, and the in-list one's.
The cost is that range pruning stops for float and double columns in
rowsets written before the flags existed, until compaction rewrites
them. A finer version would keep pruning for the operators a hidden NaN
cannot satisfy (`<`, `<=`) and disable it only for the ones it can (`>`,
`>=`, `= NaN`, `!=` against a non-NaN literal), which is the distinction
the Parquet readers already make with their own unknown-NaN-count flag.
That needs a new state carried into both the expression zone map path
and the older column predicate path, so it is a follow-up rather than
part of a correctness fix that wants backporting.
### Release note
Fixed a correctness bug where a query could silently drop rows
containing NaN when reading float or double columns from rowsets written
before NaN-aware zone maps.
### Check List (For Author)
- Test <!-- At least one of them must be included. -->
- [ ] Regression test
- [X] Unit Test
- [ ] Manual test (add detailed scripts or steps below)
- [ ] No need to test or manual test. Explain why:
- [ ] This is a refactor/code format and no logic has been changed.
- [ ] Previous test can cover this change.
- [ ] No code files have been changed.
- [ ] Other reason <!-- Add your reason? -->
`LegacyFloatZoneMapWithoutHasNanDegradesToPassAll` covers FLOAT and
DOUBLE, nullable and not: a `ZoneMapPB` with valid ordered finite bounds
and no field 8 comes back with `pass_all` set and its null flags intact,
the same bounds with `set_has_nan(false)` stay usable, and an INT zone
map without the field is unaffected.
Two existing tests build a DOUBLE `ZoneMapPB` without calling
`set_has_nan`, which now reads as legacy metadata and made their control
cases fail. Their helpers set `set_has_nan(false)`, which is what the
writer actually emits, so each test keeps testing what it was written
for.
The test also covers the all-null case the guard deliberately excludes,
and both the guard and that exclusion were checked by mutation: removing
the guard, or dropping the `has_not_null()` conjunct, each makes the new
test fail.
A reader-level fixture driving page and segment pruning would be
stronger than a `from_proto` unit test, but the honest version of it
needs a segment artifact produced by a pre-flag build, which is not
something this PR can generate.
- Behavior changed:
- [ ] No.
- [X] Yes.
Float and double columns in rowsets written before the NaN flags existed
no longer participate in range pruning. Rowsets written by current code
are unaffected, since their zone maps carry the flag.
This is a candidate for backport to the maintained 4.0 and 4.1 lines,
which are the readers that can encounter pre-4.0 rowsets.
- Does this need documentation?
- [X] No.
---
be/src/storage/index/zone_map/zone_map_index.cpp | 14 +++++
be/test/storage/segment/zone_map_index_test.cpp | 80 +++++++++++++++++++++++-
2 files changed, 92 insertions(+), 2 deletions(-)
diff --git a/be/src/storage/index/zone_map/zone_map_index.cpp
b/be/src/storage/index/zone_map/zone_map_index.cpp
index 75587761f37..401a077d130 100644
--- a/be/src/storage/index/zone_map/zone_map_index.cpp
+++ b/be/src/storage/index/zone_map/zone_map_index.cpp
@@ -78,6 +78,20 @@ Status ZoneMap::from_proto(const ZoneMapPB& zone_map, const
DataTypePtr& data_ty
};
auto field_type = data_type->get_storage_field_type();
+
+ // has_nan arrived with NaN-aware float/double zone maps, so its absence
means the writer could
+ // not report NaN and the bounds came from a comparison that never selects
one: a hidden NaN
+ // cannot be ruled out. Doris orders NaN above every other value, so `x >
c` can be true for a
+ // row that these bounds say cannot exist. Treat such a zone map as
covering everything instead
+ // of as NaN-free. A zone with no non-null value never received one, so it
has no NaN to hide;
+ // leaving it usable keeps the three ColumnPredicate checks that start
from has_not_null: the
+ // null predicates, each comparison predicate's early return, and the
in-list one's.
+ if ((field_type == FieldType::OLAP_FIELD_TYPE_FLOAT ||
+ field_type == FieldType::OLAP_FIELD_TYPE_DOUBLE) &&
+ zone_map.has_not_null() && !zone_map.has_has_nan()) {
+ zone_map_info.pass_all = true;
+ }
+
// min value and max value are valid if has_not_null is true
if (zone_map.has_not_null()) {
if (!zone_map_info.pass_all) {
diff --git a/be/test/storage/segment/zone_map_index_test.cpp
b/be/test/storage/segment/zone_map_index_test.cpp
index fb12dacf04b..85e49a1acbf 100644
--- a/be/test/storage/segment/zone_map_index_test.cpp
+++ b/be/test/storage/segment/zone_map_index_test.cpp
@@ -1014,6 +1014,74 @@ TEST_F(ColumnZoneMapTest, DoubleFiniteExtremesRoundTrip)
{
EXPECT_EQ(pzm.max_value.get<TYPE_DOUBLE>(),
std::numeric_limits<double>::max());
}
+TEST_F(ColumnZoneMapTest, LegacyFloatZoneMapWithoutHasNanDegradesToPassAll) {
+ // A float or double zone map written before has_nan existed says nothing
about NaN, and its
+ // bounds were produced by a comparison that never picks a NaN. Since
Doris sorts NaN above
+ // every other value, trusting such bounds drops NaN rows from `>` and
`>=`.
+ auto legacy_pb = [](double min_value, double max_value) {
+ ZoneMapPB pb;
+ pb.set_min(std::to_string(min_value));
+ pb.set_max(std::to_string(max_value));
+ pb.set_has_null(false);
+ pb.set_has_not_null(true);
+ pb.set_pass_all(false);
+ // Deliberately no set_has_nan / set_has_positive_inf /
set_has_negative_inf.
+ return pb;
+ };
+
+ for (const auto primitive_type : {TYPE_FLOAT, TYPE_DOUBLE}) {
+ for (bool nullable : {false, true}) {
+ auto data_type =
DataTypeFactory::instance().create_data_type(primitive_type, nullable);
+ ZoneMap zm;
+ ASSERT_TRUE(ZoneMap::from_proto(legacy_pb(1.0, 2.0), data_type,
zm).ok());
+ EXPECT_TRUE(zm.pass_all) << "type=" << primitive_type << ",
nullable=" << nullable;
+ // The null flags survive, and so does the pruning that reads only
them:
+ // eval_null_zonemap never consults pass_all. The ColumnPredicate
path in
+ // ColumnReader::_get_filtered_pages does return early on
pass_all, so it loses its
+ // IS NULL pruning for this zone map.
+ EXPECT_TRUE(zm.has_not_null);
+ EXPECT_FALSE(zm.has_null);
+
+ // The same bounds from a writer that does report the flag stay
usable.
+ auto pb = legacy_pb(1.0, 2.0);
+ pb.set_has_nan(false);
+ ZoneMap current;
+ ASSERT_TRUE(ZoneMap::from_proto(pb, data_type, current).ok());
+ EXPECT_FALSE(current.pass_all)
+ << "type=" << primitive_type << ", nullable=" << nullable;
+ if (primitive_type == TYPE_DOUBLE) {
+ EXPECT_EQ(1.0, current.min_value.get<TYPE_DOUBLE>());
+ EXPECT_EQ(2.0, current.max_value.get<TYPE_DOUBLE>());
+ } else {
+ EXPECT_EQ(1.0F, current.min_value.get<TYPE_FLOAT>());
+ EXPECT_EQ(2.0F, current.max_value.get<TYPE_FLOAT>());
+ }
+
+ // A legacy zone with no non-null value never received one, so no
NaN can hide in it.
+ // The bound text is irrelevant here: from_proto only parses it
when has_not_null.
+ auto all_null = legacy_pb(1.0, 2.0);
+ all_null.set_has_null(true);
+ all_null.set_has_not_null(false);
+ ZoneMap all_null_zm;
+ ASSERT_TRUE(ZoneMap::from_proto(all_null, data_type,
all_null_zm).ok());
+ EXPECT_FALSE(all_null_zm.pass_all)
+ << "type=" << primitive_type << ", nullable=" << nullable;
+ }
+ }
+
+ // Non-floating columns never carried the flag and are unaffected.
+ auto int_type = DataTypeFactory::instance().create_data_type(TYPE_INT,
false);
+ ZoneMapPB int_pb;
+ int_pb.set_min("1");
+ int_pb.set_max("9");
+ int_pb.set_has_null(false);
+ int_pb.set_has_not_null(true);
+ int_pb.set_pass_all(false);
+ ZoneMap int_zm;
+ ASSERT_TRUE(ZoneMap::from_proto(int_pb, int_type, int_zm).ok());
+ EXPECT_FALSE(int_zm.pass_all);
+}
+
TEST_F(ColumnZoneMapTest, LegacyUnparsableDoubleBoundDegradesToPassAll) {
auto make_zone_map = [](const std::string& min, const std::string& max) {
ZoneMapPB pb;
@@ -1022,6 +1090,9 @@ TEST_F(ColumnZoneMapTest,
LegacyUnparsableDoubleBoundDegradesToPassAll) {
pb.set_has_null(false);
pb.set_has_not_null(true);
pb.set_pass_all(false);
+ // The current writer always reports this flag; leaving it out would
mark the zone map as
+ // pre-NaN-tracking legacy metadata, which is a different test.
+ pb.set_has_nan(false);
return pb;
};
// 16g renderings of ±DBL_MAX, both of which read back as ∓inf.
@@ -1162,10 +1233,15 @@ TEST_F(ColumnZoneMapTest,
ReversedBoundsDegradeToPassAll) {
pb.set_has_null(false);
pb.set_has_not_null(true);
pb.set_pass_all(false);
+ // The current writer always reports this flag; leaving it out would
mark the zone map as
+ // pre-NaN-tracking legacy metadata, which is a different test.
+ pb.set_has_nan(false);
return pb;
};
- // What a page of only NaN leaves behind before 4.0: bounds that never
moved off the values
- // the writer starts from, and that round-trip exactly, so only the
reversal gives them away.
+ // add_values() starts each call from numeric_limits::max() and
::lowest(), and a page whose
+ // only non-null values are NaN or infinity leaves them there, so the
stored pair comes back
+ // reversed. These strings round-trip exactly, and the reversal is the
first signal either way:
+ // is_reversed runs before the flag overrides.
const std::string double_lowest = "-1.7976931348623157e+308";
const std::string double_highest = "1.7976931348623157e+308";
const std::string float_lowest = "-3.4028235e+38";
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
