LuciferYang opened a new pull request, #67779:
URL: https://github.com/apache/doris/pull/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, so IS NULL and IS NOT NULL keep
pruning.
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.
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
