Doris-Breakwater commented on issue #67773:
URL: https://github.com/apache/doris/issues/67773#issuecomment-5614088801
## Breakwater on-call analysis
**Verdict: confirmed, actionable compatibility/correctness bug.** The
proposed failure mode is supported by the code, and the cross-version read path
is reachable. I would prioritize this as a high-severity query-correctness
issue because it can silently omit qualifying rows; the remaining unknown is
prevalence, not validity.
### Verified facts
- `ZoneMapPB` is proto2, and `has_nan` is field 8. `ZoneMap::from_proto`
currently assigns `zone_map.has_nan()` without consulting `has_has_nan()`, so
absent and explicitly-false metadata collapse to the same in-memory value.
- Before #55633, the writer already instantiated zone maps for FLOAT and
DOUBLE and used the generic `std::minmax_element` path. Reproducing that exact
path with `{1.0, NaN, 2.0}` yields stored bounds `[1.0, 2.0]`; the old protobuf
cannot record the hidden NaN.
- Doris comparison semantics place NaN above finite values. Both the legacy
column-predicate path and the expression zone-map path consume the result of
`ZoneMap::from_proto`, so those finite bounds can cause a false-negative page
or segment prune for a predicate such as `d > 3.0`.
- The recent reversed-bounds protection from #67431 only detects legacy
pages whose bounds never moved (for example, NaN-only pages). Mixed finite/NaN
pages have ordinary-looking ordered bounds, so that protection does not cover
this case.
- This is not only hypothetical dead-format compatibility: the 3.1 branch
has no `has_nan` field, while the 4.0/4.1 readers have the unguarded read. The
[official 4.x upgrade
guide](https://doris.apache.org/docs/4.x/admin-manual/cluster-management/upgrade/)
explicitly covers 3.x-to-4.0 upgrades and states that an upgrade replaces
FE/BE binaries and libraries, leaving existing data files in place. Legacy
rowsets can therefore be read by an affected 4.x BE immediately after upgrade.
- The issue currently has no labels or assignee. It should be triaged under
the project's BE/storage/zone-map correctness conventions.
### Recommended fix
Handle the compatibility distinction centrally in `ZoneMap::from_proto`: for
FLOAT or DOUBLE, an absent field 8 should make range bounds unusable (for
example, set the deserialized zone map's `pass_all` state, or introduce an
explicit range-validity state). Preserve `has_null`/`has_not_null` so null-only
pruning remains available. Do not fail the scan.
Using field presence is preferable to a segment-version gate: fields 6-8
were introduced together, and the Doris writer explicitly calls
`set_has_nan(false)` for a known NaN-free zone, so `has_has_nan()`
distinguishes new known-false metadata from legacy unknown metadata without
inferring a deployment tag or version.
This needs to remain in the common deserialization/index layer so segment
pruning, page pruning, predicate elimination, and expression zone-map
evaluation inherit the conservative behavior. A consumer-only check would leave
other pruning paths exposed.
### Tests and next steps
1. Add FLOAT and DOUBLE unit cases that construct a `ZoneMapPB` with valid
ordered finite bounds and deliberately do not set field 8; after `from_proto`,
range pruning must be disabled.
2. Add the control case with `set_has_nan(false)`; its range must remain
usable. Also verify that an older non-floating zone map without field 8 remains
usable.
3. Add a reader-level regression for both page and segment pruning using
legacy metadata representing `{1.0, NaN, 2.0}` and `d > 3.0`. A segment
artifact written by a pre-#55633 build would be the strongest cross-version
regression fixture.
4. Backport the fix to maintained 4.0 and 4.1 lines, since those are the
readers that can encounter pre-4.0 rowsets. Coordinate with #67431 coverage:
the current 4.0 branch also lacks that separate reversed-bounds safeguard.
No logs or runtime profile are required to accept the bug because the
metadata compatibility failure is deterministic. To quantify exposure and
release urgency, it would still help to collect the source version(s) of
upgraded clusters, whether FLOAT/DOUBLE columns can contain NaN, and whether
all pre-4.0 rowsets have actually been rewritten by compaction. Until that
inventory exists, affected deployments should not assume compaction has removed
all legacy segments.
Breakwater-GitHub-Analysis-Slot: slot_2338064ac779
--
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]
