This is an automated email from the ASF dual-hosted git repository.
yiguolei pushed a commit to branch branch-4.1
in repository https://gitbox.apache.org/repos/asf/doris.git
The following commit(s) were added to refs/heads/branch-4.1 by this push:
new 8371e5ad2bb branch-4.1: [fix](zonemap) Treat unparsable zone map as
invalid (#67341) (#67528)
8371e5ad2bb is described below
commit 8371e5ad2bb17ced8f1ca7b02be16654f349a3ad
Author: Chenyang Sun <[email protected]>
AuthorDate: Sun Sep 6 06:16:01 2026 +0800
branch-4.1: [fix](zonemap) Treat unparsable zone map as invalid (#67341)
(#67528)
pick from master #67341
Co-authored-by: Claude Opus 5 (1M context) <[email protected]>
---
.../data_type_serde/data_type_nullable_serde.cpp | 9 ++---
be/src/storage/index/zone_map/zone_map_index.cpp | 14 +++++--
be/src/storage/segment/column_reader.cpp | 39 +++++++++++---------
be/src/storage/segment/segment.cpp | 43 ++++++++++++++++++++--
.../data_type_serde_number_test.cpp | 14 +++++++
be/test/storage/segment/zone_map_index_test.cpp | 38 +++++++++++++++++++
6 files changed, 126 insertions(+), 31 deletions(-)
diff --git a/be/src/core/data_type_serde/data_type_nullable_serde.cpp
b/be/src/core/data_type_serde/data_type_nullable_serde.cpp
index a0b6701f050..a125e722fba 100644
--- a/be/src/core/data_type_serde/data_type_nullable_serde.cpp
+++ b/be/src/core/data_type_serde/data_type_nullable_serde.cpp
@@ -565,13 +565,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 {
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 31cfacf8c45..5504b4916c9 100644
--- a/be/src/storage/index/zone_map/zone_map_index.cpp
+++ b/be/src/storage/index/zone_map/zone_map_index.cpp
@@ -55,6 +55,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()) {
@@ -70,8 +78,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);
}
}
@@ -97,8 +104,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);
}
}
}
diff --git a/be/src/storage/segment/column_reader.cpp
b/be/src/storage/segment/column_reader.cpp
index 71c0eae2546..a3fa0329b68 100644
--- a/be/src/storage/segment/column_reader.cpp
+++ b/be/src/storage/segment/column_reader.cpp
@@ -331,7 +331,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;
}
@@ -449,6 +450,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) {
@@ -519,26 +523,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));
}
}
}
diff --git a/be/src/storage/segment/segment.cpp
b/be/src/storage/segment/segment.cpp
index 21063fb3b58..c293434abee 100644
--- a/be/src/storage/segment/segment.cpp
+++ b/be/src/storage/segment/segment.cpp
@@ -148,6 +148,34 @@ Status build_segment_zonemap_context(Segment* segment,
const Schema& 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 Schema& schema,
+ const StorageReadOptions&
read_options, bool* usable) {
+ *usable = true;
+ for (size_t i = 0; i < schema.num_column_ids(); ++i) {
+ std::shared_ptr<ColumnReader> reader;
+ Status st =
segment->get_column_reader(*schema.column(schema.column_id(i)), &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()) {
@@ -435,9 +463,18 @@ Status Segment::new_iterator(SchemaSPtr schema, const
StorageReadOptions& read_o
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);
diff --git a/be/test/core/data_type_serde/data_type_serde_number_test.cpp
b/be/test/core/data_type_serde/data_type_serde_number_test.cpp
index e7d832d3dd4..9e3c40301f7 100644
--- a/be/test/core/data_type_serde/data_type_serde_number_test.cpp
+++ b/be/test/core/data_type_serde/data_type_serde_number_test.cpp
@@ -35,6 +35,7 @@
#include "core/data_type/common_data_type_test.h"
#include "core/data_type/data_type.h"
#include "core/data_type/primitive_type.h"
+#include "core/data_type_serde/data_type_nullable_serde.h"
#include "core/field.h"
#include "core/types.h"
#include "testutil/test_util.h"
@@ -547,4 +548,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
diff --git a/be/test/storage/segment/zone_map_index_test.cpp
b/be/test/storage/segment/zone_map_index_test.cpp
index bb60fd056be..d79f0f225f0 100644
--- a/be/test/storage/segment/zone_map_index_test.cpp
+++ b/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;
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
