github-actions[bot] commented on code in PR #67431:
URL: https://github.com/apache/doris/pull/67431#discussion_r3911576164
##########
be/src/storage/index/zone_map/zone_map_index.cpp:
##########
@@ -280,6 +309,11 @@ Status
TypedZoneMapIndexWriter<Type>::finish(io::FileWriter* file_writer,
index_meta->set_type(ZONE_MAP_INDEX);
ZoneMapIndexPB* meta = index_meta->mutable_zone_map_index();
// store segment zone map
+ if (_segment_zone_map.has_not_null &&
+ is_reversed(_segment_zone_map.min_value, _segment_zone_map.max_value,
+ _data_type->get_storage_field_type())) {
+ _segment_zone_map.pass_all = true;
Review Comment:
[P1] Keep segment metadata readable by older BEs
This starts writing segment-level `pass_all=true`, which makes `to_proto()`
clear both bounds. Released 4.1 readers do not have the
`segment_zone_maps_can_answer_agg()` fence added in #67341: they still select
`VStatisticsIterator` for pushed-down MIN/MAX, skip parsing the cleared strings
because `pass_all` is set, and then insert both bounds. For a non-nullable
FLOAT/DOUBLE all-NaN (or single-infinity) segment, one side remains a default
TYPE_NULL `Field`, so an older BE reading a segment produced by an upgraded BE
can consume uninitialized storage and return a wrong aggregate during rolling
upgrade. This is reachable through shared cloud segments and replica
snapshot/clone. Please keep the legacy segment encoding until every supported
reader fences `pass_all`, or version-gate this on-disk transition. The
reader-side reversal check can still repair existing metadata, and page-level
`pass_all` is already established.
##########
be/src/storage/index/zone_map/zone_map_index.cpp:
##########
@@ -65,6 +80,19 @@ Status ZoneMap::from_proto(const ZoneMapPB& zone_map, const
DataTypePtr& data_ty
auto field_type = data_type->get_storage_field_type();
// 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) {
+ parse_bound(zone_map.min(), zone_map_info.min_value);
Review Comment:
[P2] Do not invalidate flag-overridden legacy bounds
Both bounds are not semantically required when a flag replaces one side. For
example, older 4.0/4.1 metadata can store `min="-1.797693134862316e+308",
max="1", has_negative_inf=true` for `{-DBL_MAX, 1, -inf}`. The current serde
rejects that historical min as infinity, but before this change it was
deliberately skipped and replaced with `-inf`, leaving a sound `[-inf, 1]`
range. Parsing it here first now sets `pass_all` and disables pruning for the
affected page/segment map. The max side has the same regression with `+inf` or
NaN, including nullable DOUBLE. Please keep parse failure fatal to pruning only
for required sides; parse a flag-overridden side best-effort for reversal
detection, and add legacy `.16g` tests for both sides.
--
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]
