This is an automated email from the ASF dual-hosted git repository.
csun5285 pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/doris.git
The following commit(s) were added to refs/heads/master by this push:
new 5fbe3600024 [fix](zonemap) Treat unparsable zone map as invalid
(#67341)
5fbe3600024 is described below
commit 5fbe3600024aae89a4d32323f42b87fe6a24324f
Author: Chenyang Sun <[email protected]>
AuthorDate: Tue Sep 1 19:09:37 2026 +0800
[fix](zonemap) Treat unparsable zone map as invalid (#67341)
### What problem does this PR solve?
A zone map bound that fails to parse is unknown, not null -- older
versions wrote DOUBLE bounds with digits10+1 precision, so DBL_MAX reads
back as infinity and is rejected. Mark such a zone map pass_all so it
prunes nothing, instead of failing every scan that loads it.
---
.../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 | 49 ++++++++++++++++++++--
.../data_type_serde_number_test.cpp | 14 +++++++
be/test/storage/segment/zone_map_index_test.cpp | 38 +++++++++++++++++
6 files changed, 132 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 c06bcf0af82..1a2befda5ab 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
@@ -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 {
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 23e2e5a4a0f..70eb29987b1 100644
--- a/be/src/storage/index/zone_map/zone_map_index.cpp
+++ b/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);
}
}
}
diff --git a/be/src/storage/segment/column_reader.cpp
b/be/src/storage/segment/column_reader.cpp
index 2dcb761940b..d11a3798012 100644
--- a/be/src/storage/segment/column_reader.cpp
+++ b/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));
}
}
}
diff --git a/be/src/storage/segment/segment.cpp
b/be/src/storage/segment/segment.cpp
index bf61ab34dd0..a637b060961 100644
--- a/be/src/storage/segment/segment.cpp
+++ b/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);
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 41724fd3b33..e9256791f59 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
@@ -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
diff --git a/be/test/storage/segment/zone_map_index_test.cpp
b/be/test/storage/segment/zone_map_index_test.cpp
index db4b692b1f1..66ad4129f66 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]
