[fix](zonemap) Treat unparsable zone map as invalid

be/src/core/data_type_serde/data_type_nullable_serde.cpp

@@ -585,13 +585,10 @@ Status DataTypeNullableSerDe::from_string(StringRef& str, IColumn& column,
  return Status::OK();
 }
 
+// A zone map bound is never a null Field -- nullness lives in has_null/has_not_null, not in
+// min/max -- so there is no null to fall back to here: defer to the nested serde as is.
 Status DataTypeNullableSerDe::from_zonemap_string(const std::string& str, Field& field) const {
- if (!nested_serde->from_zonemap_string(str, field).ok()) {
- // fill null if fail
- field = Field();
- return Status::OK();
- }
- return Status::OK();
+ return nested_serde->from_zonemap_string(str, field);
 }
 
 Status DataTypeNullableSerDe::from_fe_string(const std::string& str, Field& field) const {

be/src/storage/index/zone_map/zone_map_index.cpp

@@ -54,6 +54,14 @@ Status ZoneMap::from_proto(const ZoneMapPB& zone_map, const DataTypePtr& data_ty
  zone_map_info.has_positive_inf = zone_map.has_positive_inf();
  zone_map_info.has_nan = zone_map.has_nan();
 
+ // A bound that fails to parse makes the zone map invalid: mark it pass_all so it prunes
+ // nothing, instead of failing the scan that loads it.
+ auto parse_bound = [&](const std::string& bound, Field& value) {
+ if (!data_type->get_serde()->from_zonemap_string(bound, value).ok()) {
+ zone_map_info.pass_all = true;
+ }
+ };
+
  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()) {
@@ -69,8 +77,7 @@ Status ZoneMap::from_proto(const ZoneMapPB& zone_map, const DataTypePtr& data_ty
  }
  } else {
  if (!zone_map_info.pass_all) {
- RETURN_IF_ERROR(data_type->get_serde()->from_zonemap_string(
- zone_map.min(), zone_map_info.min_value));
+ parse_bound(zone_map.min(), zone_map_info.min_value);
  }
  }
 
@@ -96,8 +103,7 @@ Status ZoneMap::from_proto(const ZoneMapPB& zone_map, const DataTypePtr& data_ty
  }
  } else {
  if (!zone_map_info.pass_all) {
- RETURN_IF_ERROR(data_type->get_serde()->from_zonemap_string(
- zone_map.max(), zone_map_info.max_value));
+ parse_bound(zone_map.max(), zone_map_info.max_value);
  }
  }
  }

be/src/storage/segment/column_reader.cpp

@@ -357,7 +357,8 @@ void ColumnReader::check_data_by_zone_map_for_test(const MutableColumnPtr& dst)
  ZoneMap zone_map;
  THROW_IF_ERROR(ZoneMap::from_proto(*_segment_zone_map, _data_type, zone_map));
 
- if (zone_map.has_null) {
+ // pass_all leaves min/max unset, so there is nothing to check the data against.
+ if (zone_map.has_null || zone_map.pass_all) {
  return;
  }
 
@@ -477,6 +478,9 @@ Status ColumnReader::next_batch_of_zone_map(size_t* n, MutableColumnPtr& dst) co
  // TODO: this work to get min/max value seems should only do once
  ZoneMap zone_map;
  RETURN_IF_ERROR(ZoneMap::from_proto(*_segment_zone_map, _data_type, zone_map));
+ // Segment::new_iterator does not build this iterator on an invalid zone map, whose min/max
+ // are unset and would be reported below as if they were data.
+ DORIS_CHECK(!zone_map.pass_all);
 
  dst->reserve(*n);
  if (!zone_map.has_not_null) {
@@ -559,26 +563,25 @@ Status ColumnReader::_get_filtered_pages(
  const std::vector<ZoneMapPB>& zone_maps = _zone_map_index->page_zone_maps();
  size_t page_size = _zone_map_index->num_pages();
  for (size_t i = 0; i < page_size; ++i) {
- if (zone_maps[i].pass_all()) {
+ segment_v2::ZoneMap zone_map;
+ RETURN_IF_ERROR(ZoneMap::from_proto(zone_maps[i], _data_type, zone_map));
+ // from_proto also sets pass_all when the zone map it parsed is invalid.
+ if (zone_map.pass_all) {
  page_indexes->push_back(cast_set<uint32_t>(i));
- } else {
- segment_v2::ZoneMap zone_map;
- RETURN_IF_ERROR(ZoneMap::from_proto(zone_maps[i], _data_type, zone_map));
- if (_zone_map_match_condition(zone_map, col_predicates)) {
- bool should_read = true;
- if (delete_predicates != nullptr) {
- for (auto del_pred : *delete_predicates) {
- // TODO: Both `min_value` and `max_value` should be 0 or neither should be 0.
- // So nullable only need to judge once.
- if (del_pred->evaluate_del(zone_map)) {
- should_read = false;
- break;
- }
+ } else if (_zone_map_match_condition(zone_map, col_predicates)) {
+ bool should_read = true;
+ if (delete_predicates != nullptr) {
+ for (auto del_pred : *delete_predicates) {
+ // TODO: Both `min_value` and `max_value` should be 0 or neither should be 0.
+ // So nullable only need to judge once.
+ if (del_pred->evaluate_del(zone_map)) {
+ should_read = false;
+ break;
  }
  }
- if (should_read) {
- page_indexes->push_back(cast_set<uint32_t>(i));
-}
+}
+if (should_read) {
+page_indexes->push_back(cast_set<uint32_t>(i));
  }
  }
  }

be/src/storage/segment/segment.cpp

@@ -143,6 +143,40 @@ Status build_segment_zonemap_context(Segment* segment, const ReadSchema& schema,
  return Status::OK();
 }
 
+// The statistics iterator answers pushed-down aggregates from the segment zone maps alone. An
+// invalid zone map has no min/max to answer with, so the caller has to read the data instead.
+Status segment_zone_maps_can_answer_agg(Segment* segment, const ReadSchema& schema,
+ const StorageReadOptions& read_options, bool* usable) {
+ *usable = true;
+ for (size_t ordinal = 0; ordinal < schema.num_block_columns(); ++ordinal) {
+ // The commit-tso column is only served correctly once its reader is created with the
+ // rowset's commit_tso as a const value. Creating it here without one would cache a reader
+ // that hands every later read the on-disk placeholder instead.
+ if (static_cast<int32_t>(ordinal) == schema.commit_tso_ordinal()) {
+ continue;
+ }
+ std::shared_ptr<ColumnReader> reader;
+ Status st = segment->get_column_reader(*schema.column(ordinal), &reader, read_options.stats,
+ &read_options.io_ctx);
+ if (st.is<ErrorCode::NOT_FOUND>()) {
+ continue;
+ }
+ RETURN_IF_ERROR(st);
+ // Columns without a zone map keep the existing behaviour: the statistics iterator reports
+ // the missing zone map itself.
+ if (reader == nullptr || !reader->has_zone_map()) {
+ continue;
+ }
+ ZoneMap zone_map;
+ RETURN_IF_ERROR(reader->get_segment_zone_map(&zone_map));
+ if (zone_map.pass_all) {
+ *usable = false;
+ return Status::OK();
+ }
+ }
+ return Status::OK();
+}
+
 void fill_missing_decimal_precision(const TabletColumn& column, ColumnMetaPB* meta) {
  auto meta_type = static_cast<FieldType>(meta->type());
  if (meta_type != column.type()) {
@@ -469,9 +503,18 @@ Status Segment::new_iterator(ReadSchemaSPtr schema, const StorageReadOptions& re
  RETURN_IF_ERROR(load_index(read_options.stats, &read_options.io_ctx));
  }
 
- if (read_options.delete_condition_predicates->num_of_column_predicate() == 0 &&
- read_options.push_down_agg_type_opt != TPushAggOp::NONE &&
- read_options.push_down_agg_type_opt != TPushAggOp::COUNT_ON_INDEX) {
+ bool use_statistics_iterator =
+ read_options.delete_condition_predicates->num_of_column_predicate() == 0 &&
+ read_options.push_down_agg_type_opt != TPushAggOp::NONE &&
+ read_options.push_down_agg_type_opt != TPushAggOp::COUNT_ON_INDEX;
+ // COUNT only fills defaults, every other pushed-down aggregate reads min/max out of the
+ // segment zone maps.
+ if (use_statistics_iterator && read_options.push_down_agg_type_opt != TPushAggOp::COUNT) {
+ bool usable = false;
+ RETURN_IF_ERROR(segment_zone_maps_can_answer_agg(this, *schema, read_options, &usable));
+ use_statistics_iterator = usable;
+ }
+ if (use_statistics_iterator) {
  iter->reset(new_vstatistics_iterator(this->shared_from_this(), *schema));
  } else {
  *iter = std::make_unique<SegmentIterator>(this->shared_from_this(), schema);

be/test/core/data_type_serde/data_type_serde_number_test.cpp

@@ -39,6 +39,7 @@
 #include "core/data_type_serde/data_type_date_or_datetime_serde.h"
 #include "core/data_type_serde/data_type_datetimev2_serde.h"
 #include "core/data_type_serde/data_type_datev2_serde.h"
+#include "core/data_type_serde/data_type_nullable_serde.h"
 #include "core/field.h"
 #include "core/types.h"
 #include "testutil/test_util.h"
@@ -793,4 +794,17 @@ TEST_F(DataTypeNumberSerDeTest, OlapStringRoundTripFloatExtremes) {
  check_float(0.0f);
 }
 
+TEST_F(DataTypeNumberSerDeTest, NullableZonemapStringPropagatesParseError) {
+ DataTypeNullableSerDe nullable_serde(serde_float64);
+
+ Field field;
+ EXPECT_FALSE(nullable_serde.from_zonemap_string("1.797693134862316e+308", field).ok());
+ EXPECT_FALSE(nullable_serde.from_zonemap_string("-1.797693134862316e+308", field).ok());
+ EXPECT_FALSE(nullable_serde.from_zonemap_string("not-a-double", field).ok());
+
+ Field parsed;
+ ASSERT_TRUE(nullable_serde.from_zonemap_string("1.7976931348623157e+308", parsed).ok());
+ EXPECT_EQ(parsed.get<TYPE_DOUBLE>(), std::numeric_limits<double>::max());
+}
+
 } // namespace doris

be/test/storage/segment/zone_map_index_test.cpp

@@ -943,6 +943,44 @@ TEST_F(ColumnZoneMapTest, DoubleFiniteExtremesRoundTrip) {
  EXPECT_EQ(pzm.max_value.get<TYPE_DOUBLE>(), std::numeric_limits<double>::max());
 }
 
+TEST_F(ColumnZoneMapTest, LegacyUnparsableDoubleBoundDegradesToPassAll) {
+ auto make_zone_map = [](const std::string& min, const std::string& max) {
+ ZoneMapPB pb;
+ pb.set_min(min);
+ pb.set_max(max);
+ pb.set_has_null(false);
+ pb.set_has_not_null(true);
+ pb.set_pass_all(false);
+ return pb;
+ };
+ // 16g renderings of ±DBL_MAX, both of which read back as ∓inf.
+ const std::string legacy_min = "-1.797693134862316e+308";
+ const std::string legacy_max = "1.797693134862316e+308";
+ const std::string exact_min = "-1.7976931348623157e+308";
+ const std::string exact_max = "1.7976931348623157e+308";
+
+ for (bool nullable : {false, true}) {
+ auto data_type = DataTypeFactory::instance().create_data_type(TYPE_DOUBLE, nullable);
+
+ for (const auto& pb :
+ {make_zone_map(legacy_min, legacy_max), make_zone_map(legacy_min, exact_max),
+ make_zone_map(exact_min, legacy_max)}) {
+ ZoneMap zm;
+ ASSERT_TRUE(ZoneMap::from_proto(pb, data_type, zm).ok()) << "nullable=" << nullable;
+ EXPECT_TRUE(zm.pass_all) << "nullable=" << nullable << ", min='" << pb.min()
+ << "', max='" << pb.max() << "'";
+ EXPECT_TRUE(zm.has_not_null);
+ EXPECT_FALSE(zm.has_null);
+ }
+
+ ZoneMap zm;
+ ASSERT_TRUE(ZoneMap::from_proto(make_zone_map(exact_min, exact_max), data_type, zm).ok());
+ EXPECT_FALSE(zm.pass_all) << "nullable=" << nullable;
+ EXPECT_EQ(zm.min_value.get<TYPE_DOUBLE>(), std::numeric_limits<double>::lowest());
+ EXPECT_EQ(zm.max_value.get<TYPE_DOUBLE>(), std::numeric_limits<double>::max());
+ }
+}
+
 TabletColumnPtr create_timestamptz_column(int32_t id, bool is_nullable) {
  auto column = std::make_shared<TabletColumn>();
  column->_unique_id = id;