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 b0f266a8ebc [fix](zonemap) Do not trust a cut string bound to prune or
to answer MIN/MAX (#67642)
b0f266a8ebc is described below
commit b0f266a8ebc1a333f806831f5d2209f837fda5d5
Author: Chenyang Sun <[email protected]>
AuthorDate: Thu Sep 17 21:31:50 2026 +0800
[fix](zonemap) Do not trust a cut string bound to prune or to answer
MIN/MAX (#67642)
1. Write side: a max cut to 512 bytes was raised with str[511] += 1. A
string column holds arbitrary bytes, so a last byte of 0xff wraps to
0x00 and leaves the max below the rows it covers — pruning then skips
pages that do hold matching rows. The raise now carries into the
preceding byte until one does not wrap.
2. Read side: a max of all 0xff carries past its first byte and ends up
all zero, standing above nothing. ZoneMap::from_proto() spots that and
turns pass_all on for the zone, giving up its range instead of ruling
rows out with it — which also covers segments written before this fix.
3. MIN/MAX push-down: a cut bound is not a value the column holds (the
min is a prefix, the max is that prefix raised), so
segment_zone_maps_can_answer_agg() rejects a string bound reaching the
512-byte cut and reads the rows instead. The FE length blacklist is
dropped with it — it was both too strict (a VARCHAR(65533) of short
values was never pushed down) and too loose (a VARCHAR(512) filled to
512 bytes is cut just the same, yet was answered with a value never
inserted).
4. Switch: enable_pushdown_string_minmax → force_pushdown_zonemap_minmax
(old name kept as an alias), now meaning "force MIN/MAX onto the zone
map even when its bound is not a value the data holds right now" — a cut
bound, or one still covering rows a delete predicate removed. Statistics
collection turns it on, every other query leaves it off. It applies to
MIN/MAX only; COUNT and MIX keep the delete-predicate guard. The new
thrift field defaults to false, so an old FE leaves BE behaving as
before.
---
be/src/storage/index/zone_map/zone_map_index.cpp | 20 +-
be/src/storage/segment/column_reader.cpp | 3 -
be/src/storage/segment/segment.cpp | 45 +++--
be/test/exec/scan/vgeneric_iterators_test.cpp | 202 +++++++++++++++++++++
be/test/storage/segment/zone_map_index_test.cpp | 129 +++++++++++++
.../rules/implementation/AggregateStrategies.java | 13 +-
.../java/org/apache/doris/qe/SessionVariable.java | 18 +-
.../doris/statistics/util/StatisticsUtil.java | 2 +-
gensrc/thrift/PaloInternalService.thrift | 6 +
.../explain/test_pushdown_zonemap_minmax.out | 19 ++
.../explain/test_pushdown_zonemap_minmax.groovy | 102 +++++++++++
.../suites/statistics/analyze_stats.groovy | 7 +-
12 files changed, 523 insertions(+), 43 deletions(-)
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 401a077d130..a44f9eac271 100644
--- a/be/src/storage/index/zone_map/zone_map_index.cpp
+++ b/be/src/storage/index/zone_map/zone_map_index.cpp
@@ -38,6 +38,7 @@
#include "storage/segment/encoding_info.h"
#include "storage/tablet/tablet_schema.h"
#include "storage/types.h"
+#include "storage/utils.h"
#include "util/slice.h"
#include "util/unaligned.h"
@@ -99,6 +100,14 @@ Status ZoneMap::from_proto(const ZoneMapPB& zone_map, const
DataTypePtr& data_ty
parse_bound(zone_map.max(), zone_map_info.max_value);
}
+ // A max of all 0xff carries past its first byte and ends up all zero,
which stands above
+ // nothing. Give up the range instead of ruling out rows with it.
+ if (!zone_map_info.pass_all && is_string_type(field_type) &&
+ zone_map.max().size() == MAX_ZONE_MAP_INDEX_SIZE &&
+ zone_map.max().find_first_not_of('\0') == std::string::npos) {
+ zone_map_info.pass_all = true;
+ }
+
// NaN and infinity only set the flags below, never min/max, so a page
holding nothing
// else leaves both at the values add_values() starts from: min =
DBL_MAX and
// max = -DBL_MAX, neither of which is a value in the page.
@@ -246,11 +255,18 @@ void
TypedZoneMapIndexWriter<Type>::modify_index_before_flush(
// slightly larger than any real string that shares the same 512-byte
prefix, ensuring no false negatives —
// the zone map will never incorrectly skip a page that contains matching
data.
//
- // In UTF8 encoding, here do not appear 0xff in last byte
+ // A string column holds arbitrary bytes, so the last byte can be 0xff.
Adding one to it wraps
+ // to 0x00 and leaves a max below the data, so carry into the byte before
it.
if constexpr (Type == TYPE_CHAR || Type == TYPE_VARCHAR || Type ==
TYPE_STRING) {
auto& str = zone_map.max_value.get<Type>();
if (str.size() == MAX_ZONE_MAP_INDEX_SIZE) {
- str[str.size() - 1] += 1;
+ for (size_t i = str.size(); i > 0; --i) {
+ auto byte = static_cast<uint8_t>(str[i - 1]) + 1;
+ str[i - 1] = static_cast<char>(byte);
+ if (static_cast<uint8_t>(byte) != 0) {
+ break;
+ }
+ }
}
}
}
diff --git a/be/src/storage/segment/column_reader.cpp
b/be/src/storage/segment/column_reader.cpp
index 765b79324ab..f6703586859 100644
--- a/be/src/storage/segment/column_reader.cpp
+++ b/be/src/storage/segment/column_reader.cpp
@@ -715,9 +715,6 @@ 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) {
diff --git a/be/src/storage/segment/segment.cpp
b/be/src/storage/segment/segment.cpp
index 1eef27b7667..9d7ae1563cb 100644
--- a/be/src/storage/segment/segment.cpp
+++ b/be/src/storage/segment/segment.cpp
@@ -143,8 +143,14 @@ 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.
+// Whether to force MIN/MAX onto the zone map when its bound is not a value
the data holds now: a
+// cut string bound, or one covering rows a delete predicate removed.
Statistics collection sets it.
+// MIN/MAX is the only aggregate this can force, because it is the only one
that reads the bounds.
+bool pushdown_zonemap_minmax_forced(const StorageReadOptions& read_options) {
+ return read_options.push_down_agg_type_opt == TPushAggOp::MINMAX &&
+
read_options.runtime_state->query_options().force_pushdown_zonemap_minmax;
+}
+
Status segment_zone_maps_can_answer_agg(Segment* segment, const ReadSchema&
schema,
const StorageReadOptions&
read_options, bool* usable) {
*usable = true;
@@ -169,10 +175,26 @@ Status segment_zone_maps_can_answer_agg(Segment* segment,
const ReadSchema& sche
}
ZoneMap zone_map;
RETURN_IF_ERROR(reader->get_segment_zone_map(&zone_map));
+
+ // The zone map gave up its range, so it has no min/max left to answer
with.
if (zone_map.pass_all) {
*usable = false;
return Status::OK();
}
+
+ // Only a string bound is cut at MAX_ZONE_MAP_INDEX_SIZE, and a column
of nothing but
+ // nulls stored no bound to look at.
+ if (!is_string_type(schema.column(ordinal)->type()) ||
!zone_map.has_not_null) {
+ continue;
+ }
+
+ // A cut bound is not a value the column holds: the min is a prefix of
the smallest value
+ // and the max was raised past the largest one. Neither can answer
MIN()/MAX().
+ if (zone_map.min_value.as_string_view().size() >=
MAX_ZONE_MAP_INDEX_SIZE ||
+ zone_map.max_value.as_string_view().size() >=
MAX_ZONE_MAP_INDEX_SIZE) {
+ *usable = false;
+ return Status::OK();
+ }
}
return Status::OK();
}
@@ -503,16 +525,17 @@ Status Segment::new_iterator(ReadSchemaSPtr schema, const
StorageReadOptions& re
RETURN_IF_ERROR(load_index(read_options.stats, &read_options.io_ctx));
}
+ // COUNT and MIX report the segment row count, which a delete predicate
makes wrong whatever
+ // the zone map bounds hold, so they keep the guard below even when the
switch is on.
+ const auto agg = read_options.push_down_agg_type_opt;
+ const bool forced = pushdown_zonemap_minmax_forced(read_options);
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;
+ agg != TPushAggOp::NONE && agg != TPushAggOp::COUNT_ON_INDEX &&
+ (forced ||
read_options.delete_condition_predicates->num_of_column_predicate() == 0);
+ // COUNT only fills defaults, every other aggregate reads min/max out of
the zone maps.
+ if (use_statistics_iterator && !forced && agg != TPushAggOp::COUNT) {
+ RETURN_IF_ERROR(segment_zone_maps_can_answer_agg(this, *schema,
read_options,
+
&use_statistics_iterator));
}
if (use_statistics_iterator) {
iter->reset(new_vstatistics_iterator(this->shared_from_this(),
*schema));
diff --git a/be/test/exec/scan/vgeneric_iterators_test.cpp
b/be/test/exec/scan/vgeneric_iterators_test.cpp
index f1b0a1e3c85..d8f1a277efa 100644
--- a/be/test/exec/scan/vgeneric_iterators_test.cpp
+++ b/be/test/exec/scan/vgeneric_iterators_test.cpp
@@ -32,7 +32,12 @@
#include "gtest/gtest_pred_impl.h"
#include "io/fs/file_writer.h"
#include "io/fs/local_file_system.h"
+#include "runtime/runtime_state.h"
#include "storage/olap_common.h"
+#include "storage/olap_define.h"
+#include "storage/olap_tuple.h"
+#include "storage/predicate/block_column_predicate.h"
+#include "storage/predicate/null_predicate.h"
#include "storage/row_cursor.h"
#include "storage/schema.h"
#include "storage/segment/column_reader.h"
@@ -184,6 +189,203 @@ TEST(VGenericIteratorsTest,
StatisticsIteratorPreservesNullForNullableChar) {
ASSERT_TRUE(fs->delete_directory(test_dir).ok());
}
+// A string zone map bound is cut to 512 bytes, and a cut bound is not a value
the column holds:
+// the min is a prefix of the smallest value and the max was raised past the
largest one. FE pushes
+// MIN/MAX down for every string column, so the segment is the one that has to
notice and hand the
+// query back to a normal read.
+class StatisticsIteratorStringBoundsTest : public testing::Test {
+protected:
+ static constexpr auto kTestDir = "./ut_dir/statistics_string_bounds_test";
+
+ void SetUp() override {
+ _fs = io::global_local_filesystem();
+ ASSERT_TRUE(_fs->delete_directory(kTestDir).ok());
+ ASSERT_TRUE(_fs->create_directory(kTestDir).ok());
+ }
+ void TearDown() override {
EXPECT_TRUE(_fs->delete_directory(kTestDir).ok()); }
+
+ static TabletSchemaSPtr make_schema() {
+ auto tablet_schema = std::make_shared<TabletSchema>();
+ TabletColumn key;
+ key.set_name("c1");
+ key.set_unique_id(0);
+ key.set_type(FieldType::OLAP_FIELD_TYPE_INT);
+ key.set_length(4);
+ key.set_index_length(4);
+ key.set_is_key(true);
+ key.set_is_nullable(false);
+ tablet_schema->append_column(key);
+
+ TabletColumn value;
+ value.set_name("c2");
+ value.set_unique_id(1);
+ value.set_type(FieldType::OLAP_FIELD_TYPE_VARCHAR);
+ value.set_length(65535);
+ value.set_is_key(false);
+ value.set_is_nullable(false);
+
value.set_aggregation_method(FieldAggregationMethod::OLAP_FIELD_AGGREGATION_NONE);
+ tablet_schema->append_column(value);
+ tablet_schema->set_storage_page_size(4096);
+ return tablet_schema;
+ }
+
+ // Writes one segment holding `values` in the VARCHAR column and returns
the iterator that the
+ // pushed-down `agg` would run on. `accept_cut_bound` is what statistics
collection sets:
+ // it takes an inexact min/max as an approximation instead of reading the
data.
+ // `with_delete` adds a delete predicate, which leaves the zone map
covering removed rows.
+ std::unique_ptr<RowwiseIterator> pushdown_iterator_for(
+ const std::string& name, const std::vector<std::string>& values,
+ bool accept_cut_bound = false, bool with_delete = false,
+ TPushAggOp::type agg = TPushAggOp::MINMAX) {
+ auto tablet_schema = make_schema();
+ const std::string segment_path = std::string(kTestDir) + "/" + name +
".dat";
+
+ io::FileWriterPtr file_writer;
+ EXPECT_TRUE(_fs->create_file(segment_path, &file_writer).ok());
+ VerticalSegmentWriterOptions writer_options;
+ writer_options.num_rows_per_block = 1024;
+ TestVerticalSegmentWriter writer(file_writer.get(), 0, tablet_schema,
nullptr, nullptr,
+ writer_options, nullptr);
+ EXPECT_TRUE(writer.init().ok());
+
+ RowCursor row;
+ OlapTuple tuple;
+ for (size_t i = 0; i < tablet_schema->num_columns(); ++i) {
+ tuple.add_null();
+ }
+ EXPECT_EQ(Status::OK(), row.init(tablet_schema, tuple));
+ for (size_t i = 0; i < values.size(); ++i) {
+ row.mutable_field(0) =
Field::create_field<TYPE_INT>(static_cast<int32_t>(i));
+ row.mutable_field(1) =
Field::create_field<TYPE_STRING>(String(values[i]));
+ EXPECT_TRUE(writer.append_row(row).ok());
+ }
+ uint64_t file_size = 0;
+ uint64_t index_size = 0;
+ EXPECT_TRUE(writer.finalize_columns(&index_size).ok());
+ EXPECT_TRUE(writer.finalize_footer(&file_size).ok());
+ EXPECT_TRUE(file_writer->close().ok());
+
+ std::shared_ptr<segment_v2::Segment> segment;
+ EXPECT_TRUE(segment_v2::Segment::open(_fs, segment_path, 100, 0,
RowsetId {.version = 1},
+ tablet_schema,
io::FileReaderOptions {}, &segment)
+ .ok());
+
+ std::vector<ColumnId> column_ids {0, 1};
+ // VStatisticsIterator keeps a reference to the schema, so it has to
outlive the iterator.
+ auto schema = std::make_shared<ReadSchema>(
+ project_columns_by_ordinal(tablet_schema->columns(),
column_ids));
+ StorageReadOptions read_options;
+ read_options.push_down_agg_type_opt = agg;
+ read_options.stats = &_stats;
+ read_options.tablet_schema = tablet_schema;
+
+ if (with_delete) {
+ auto del_pred = NullPredicate::create_shared(0, "c1", true,
PrimitiveType::TYPE_INT);
+ read_options.delete_condition_predicates->add_column_predicate(
+ SingleColumnBlockPredicate::create_unique(del_pred));
+ }
+
+ auto state = std::make_unique<RuntimeState>();
+ TQueryOptions query_options;
+ query_options.__set_force_pushdown_zonemap_minmax(accept_cut_bound);
+ state->set_query_options(query_options);
+ read_options.runtime_state = state.get();
+ // The iterator keeps a copy of read_options, so the state has to
outlive it.
+ _states.push_back(std::move(state));
+ _schemas.push_back(schema);
+
+ std::unique_ptr<RowwiseIterator> iter;
+ EXPECT_TRUE(segment->new_iterator(schema, read_options, &iter).ok());
+ return iter;
+ }
+
+ std::shared_ptr<io::FileSystem> _fs;
+ OlapReaderStatistics _stats;
+ std::vector<std::unique_ptr<RuntimeState>> _states;
+ std::vector<ReadSchemaSPtr> _schemas;
+};
+
+TEST_F(StatisticsIteratorStringBoundsTest, ShortBoundsAnswerFromTheZoneMap) {
+ // Every value fits well inside the 512-byte bound, so the stored min/max
are the real ones.
+ auto iter = pushdown_iterator_for("short", {"aaa", "bbb", "ccc"});
+ EXPECT_NE(dynamic_cast<VStatisticsIterator*>(iter.get()), nullptr)
+ << "exact bounds can answer MIN/MAX without reading the data";
+}
+
+TEST_F(StatisticsIteratorStringBoundsTest, CutBoundsFallBackToReadingTheData) {
+ // The longest value runs past the 512-byte cut, so the stored max is a
raised prefix and not a
+ // value in the column. Answering MIN/MAX from it would return a string
the table never held.
+ auto iter = pushdown_iterator_for("cut", {"aaa", "bbb", std::string(600,
'c')});
+ EXPECT_EQ(dynamic_cast<VStatisticsIterator*>(iter.get()), nullptr)
+ << "a cut bound is not a value from the data, so the query has to
read the rows";
+}
+
+// A VARCHAR(512) column full to its declared length was cut too, and FE used
to push MIN/MAX down
+// for it because the length is not over 512.
+TEST_F(StatisticsIteratorStringBoundsTest, BoundsCutExactlyAtTheLimitFallBack)
{
+ auto iter = pushdown_iterator_for("exact", {"aaa",
std::string(MAX_ZONE_MAP_INDEX_SIZE, 'z')});
+ EXPECT_EQ(dynamic_cast<VStatisticsIterator*>(iter.get()), nullptr);
+}
+
+// Statistics collection only needs an approximation, and reading the data
instead would scan the
+// whole table. It keeps the statistics iterator even when the stored bounds
were cut.
+TEST_F(StatisticsIteratorStringBoundsTest,
CutBoundsAnswerWhenTheCallerTakesAnApproximation) {
+ auto iter = pushdown_iterator_for("cut_approx", {"aaa", "bbb",
std::string(600, 'c')},
+ /*accept_cut_bound=*/true);
+ EXPECT_NE(dynamic_cast<VStatisticsIterator*>(iter.get()), nullptr)
+ << "statistics collection reads the cut bound rather than scanning
the rows";
+}
+
+// A max raised from 0xff wraps to 0x00, so the read side turns pass_all on
for that zone. The
+// bounds were parsed before that happened, so statistics collection still
reads them.
+TEST_F(StatisticsIteratorStringBoundsTest,
PassAllZoneMapAnswersWhenApproximationIsAccepted) {
+ std::string wrapping(MAX_ZONE_MAP_INDEX_SIZE - 1, 'a');
+ wrapping.push_back(static_cast<char>(0xff));
+ auto iter = pushdown_iterator_for("pass_all_approx", {"aaa", wrapping},
+ /*accept_cut_bound=*/true);
+ EXPECT_NE(dynamic_cast<VStatisticsIterator*>(iter.get()), nullptr)
+ << "a zone map that gave up its range on read still carries the
bounds it parsed";
+
+ Block block;
+ for (const auto& column : iter->schema().columns()) {
+ auto data_type = column->get_vec_type();
+ block.insert(ColumnWithTypeAndName(data_type->create_column(),
data_type, column->name()));
+ }
+ EXPECT_TRUE(iter->next_batch(&block).ok()) << "reading the bounds must not
trip an assertion";
+}
+
+// With the switch off the same zone map sends the query back to the rows.
+TEST_F(StatisticsIteratorStringBoundsTest,
PassAllZoneMapFallsBackToReadingTheData) {
+ std::string wrapping(MAX_ZONE_MAP_INDEX_SIZE - 1, 'a');
+ wrapping.push_back(static_cast<char>(0xff));
+ auto iter = pushdown_iterator_for("pass_all_exact", {"aaa", wrapping});
+ EXPECT_EQ(dynamic_cast<VStatisticsIterator*>(iter.get()), nullptr);
+}
+
+// A delete predicate leaves the zone map covering rows that are gone, so its
min/max may name a
+// value the table no longer holds. That is a real answer for every query but
statistics
+// collection, which takes the approximation to avoid scanning the table.
+TEST_F(StatisticsIteratorStringBoundsTest,
DeletePredicateFallsBackToReadingTheData) {
+ auto iter = pushdown_iterator_for("del_exact", {"aaa", "bbb"},
/*accept_cut_bound=*/false,
+ /*with_delete=*/true);
+ EXPECT_EQ(dynamic_cast<VStatisticsIterator*>(iter.get()), nullptr)
+ << "a deleted row may still sit inside the zone map bounds";
+}
+
+TEST_F(StatisticsIteratorStringBoundsTest,
DeletePredicateAnswersWhenApproximationIsAccepted) {
+ auto iter = pushdown_iterator_for("del_approx", {"aaa", "bbb"},
/*accept_cut_bound=*/true,
+ /*with_delete=*/true);
+ EXPECT_NE(dynamic_cast<VStatisticsIterator*>(iter.get()), nullptr)
+ << "statistics collection keeps the zone map even with a delete
predicate";
+}
+
+TEST_F(StatisticsIteratorStringBoundsTest,
CountKeepsTheDeletePredicateGuardWhenForced) {
+ auto iter = pushdown_iterator_for("count_del", {"aaa", "bbb"},
/*accept_cut_bound=*/true,
+ /*with_delete=*/true, TPushAggOp::COUNT);
+ EXPECT_EQ(dynamic_cast<VStatisticsIterator*>(iter.get()), nullptr)
+ << "COUNT reports the segment row count, which still counts the
deleted rows";
+}
+
TEST(VGenericIteratorsTest, Union) {
auto schema = create_schema();
auto output_schema = std::make_shared<ReadSchema>(schema);
diff --git a/be/test/storage/segment/zone_map_index_test.cpp
b/be/test/storage/segment/zone_map_index_test.cpp
index 85e49a1acbf..78efd889c27 100644
--- a/be/test/storage/segment/zone_map_index_test.cpp
+++ b/be/test/storage/segment/zone_map_index_test.cpp
@@ -1688,5 +1688,134 @@ TEST_F(ColumnZoneMapTest, EmbeddedNulKeepsStringBound) {
test_embedded_nul_bound<TYPE_CHAR>("embedded_nul_char",
/*bound_is_cut=*/true);
}
+// The writer raises the last byte of every cut max, including one that wraps.
Storing the bound
+// anyway keeps it available to a reader that only needs an approximation, and
the read side is
+// where the wrap is caught.
+TEST_F(ColumnZoneMapTest, WriterRaisesEveryCutMax) {
+ auto data_type = DataTypeFactory::instance().create_data_type(TYPE_STRING,
true, 0, 0, -1);
+ TabletColumnPtr tab_col = create_string_key(0);
+
+ struct Raised {
+ std::string max;
+ bool pass_all;
+ };
+ auto raise_max = [&](const std::string& value) {
+ std::unique_ptr<ZoneMapIndexWriter> writer;
+ EXPECT_TRUE(ZoneMapIndexWriter::create(data_type, tab_col.get(),
writer).ok());
+ segment_v2::ZoneMap zone_map;
+ zone_map.min_value = Field::create_field<TYPE_STRING>(value);
+ zone_map.max_value = Field::create_field<TYPE_STRING>(value);
+ zone_map.has_not_null = true;
+ writer->modify_index_before_flush(zone_map);
+ return Raised {zone_map.max_value.get<TYPE_STRING>(),
zone_map.pass_all};
+ };
+
+ // 511 'a' then 0xff: adding one to the last byte wraps it, so the carry
goes into the byte
+ // before it and the max still stands above the value it covers.
+ std::string trailing_ff(MAX_ZONE_MAP_INDEX_SIZE - 1, 'a');
+ trailing_ff.push_back(static_cast<char>(0xff));
+ const auto carried = raise_max(trailing_ff);
+ EXPECT_FALSE(carried.pass_all);
+ EXPECT_EQ(std::string(MAX_ZONE_MAP_INDEX_SIZE - 2, 'a') + "b" + '\0',
carried.max);
+ EXPECT_GT(carried.max, trailing_ff) << "max must stay above the value it
covers";
+
+ // A max that is 0xff all the way down carries past its first byte, so
every byte ends at
+ // 0x00. The read side spots that the same way it spots an old wrap.
+ const auto all_ff = raise_max(std::string(MAX_ZONE_MAP_INDEX_SIZE,
static_cast<char>(0xff)));
+ EXPECT_FALSE(all_ff.pass_all);
+ EXPECT_EQ(std::string(MAX_ZONE_MAP_INDEX_SIZE, '\0'), all_ff.max);
+
+ // A plain max gets the plain raise.
+ const std::string plain(MAX_ZONE_MAP_INDEX_SIZE, 'x');
+ const auto raised = raise_max(plain);
+ EXPECT_FALSE(raised.pass_all);
+ EXPECT_EQ(std::string(MAX_ZONE_MAP_INDEX_SIZE - 1, 'x') + "y", raised.max);
+ EXPECT_GT(raised.max, plain) << "max must stay above the value it covers";
+
+ // The 512-byte cut is a plain byte cut, so it can land inside a character
and leave a bound
+ // that is not UTF-8. The raise still stands above every value sharing the
prefix, so the zone
+ // keeps its range: long CJK text must not lose pruning over a split
character.
+ std::string cut_mid_char(MAX_ZONE_MAP_INDEX_SIZE - 2, 'a');
+ cut_mid_char.push_back(static_cast<char>(0xe4)); // first byte of a
three-byte character
+ cut_mid_char.push_back(static_cast<char>(0xb8));
+ const auto mid_char = raise_max(cut_mid_char);
+ EXPECT_FALSE(mid_char.pass_all);
+ EXPECT_GT(mid_char.max, cut_mid_char);
+
+ // Same when the cut keeps only the first byte of that character.
+ std::string cut_after_lead(MAX_ZONE_MAP_INDEX_SIZE - 1, 'a');
+ cut_after_lead.push_back(static_cast<char>(0xe4));
+ const auto after_lead = raise_max(cut_after_lead);
+ EXPECT_FALSE(after_lead.pass_all);
+ EXPECT_GT(after_lead.max, cut_after_lead);
+
+ // A character that ends right on the cut is whole.
+ std::string cut_on_boundary(MAX_ZONE_MAP_INDEX_SIZE - 3, 'a');
+ cut_on_boundary.push_back(static_cast<char>(0xe4));
+ cut_on_boundary.push_back(static_cast<char>(0xb8));
+ cut_on_boundary.push_back(static_cast<char>(0xad));
+ const auto whole_raised = raise_max(cut_on_boundary);
+ EXPECT_FALSE(whole_raised.pass_all);
+ EXPECT_EQ(static_cast<unsigned char>(whole_raised.max.back()), 0xae);
+ EXPECT_GT(whole_raised.max, cut_on_boundary);
+}
+
+// The writer raises every cut max, so a max that came from 0xff wrapped to
0x00 and now sits
+// below the rows it covers. The read side has to spot that and give up the
range, or those rows
+// stay invisible. Segments written before this carry the same wrapped max.
+TEST_F(ColumnZoneMapTest,
FromProtoGivesUpTheRangeForAMaxThatCarriedPastItsEnd) {
+ auto data_type = DataTypeFactory::instance().create_data_type(TYPE_STRING,
true, 0, 0, -1);
+
+ auto reads_back_as_pass_all = [&](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);
+ ZoneMap zone_map;
+ EXPECT_TRUE(ZoneMap::from_proto(pb, data_type, zone_map).ok());
+ return zone_map.pass_all;
+ };
+
+ // A carry that stopped inside the bound left 0x00 in the last byte, but
an earlier byte went
+ // up, so the max still stands above the rows. Only an all-zero max covers
nothing.
+ std::string carried(MAX_ZONE_MAP_INDEX_SIZE - 2, 'a');
+ carried.push_back('b');
+ carried.push_back('\0');
+ EXPECT_FALSE(reads_back_as_pass_all("aaa", carried));
+
+ // A max raised from a plain byte keeps its range.
+ EXPECT_FALSE(
+ reads_back_as_pass_all("aaa", std::string(MAX_ZONE_MAP_INDEX_SIZE
- 1, 'x') + "y"));
+
+ // So does one raised from a whole character.
+ std::string whole_raised(MAX_ZONE_MAP_INDEX_SIZE - 3, 'a');
+ whole_raised.push_back(static_cast<char>(0xe4));
+ whole_raised.push_back(static_cast<char>(0xb8));
+ whole_raised.push_back(static_cast<char>(0xae));
+ EXPECT_FALSE(reads_back_as_pass_all("aaa", whole_raised));
+
+ // A cut that split a character in half still raised the last byte, so the
max stands above
+ // every value sharing the prefix. Long CJK text must not lose pruning
over that.
+ std::string cut_raised(MAX_ZONE_MAP_INDEX_SIZE - 2, 'a');
+ cut_raised.push_back(static_cast<char>(0xe4));
+ cut_raised.push_back(static_cast<char>(0xb9));
+ EXPECT_FALSE(reads_back_as_pass_all("aaa", cut_raised));
+
+ // A max ending in 0xff was never raised into one, so it keeps its range.
+ std::string ends_with_ff(MAX_ZONE_MAP_INDEX_SIZE - 1, 'a');
+ ends_with_ff.push_back(static_cast<char>(0xff));
+ EXPECT_FALSE(reads_back_as_pass_all("aaa", ends_with_ff));
+
+ // A max of all 0xff carries through every byte and ends up all zero,
covering nothing.
+ EXPECT_TRUE(reads_back_as_pass_all("aaa",
std::string(MAX_ZONE_MAP_INDEX_SIZE, '\0')));
+
+ // A max shorter than the cut was never raised, so it is exact whatever
bytes it holds.
+ std::string short_ff = "abc";
+ short_ff.push_back(static_cast<char>(0xff));
+ EXPECT_FALSE(reads_back_as_pass_all("abc", short_ff));
+}
+
} // namespace segment_v2
} // namespace doris
diff --git
a/fe/fe-core/src/main/java/org/apache/doris/nereids/rules/implementation/AggregateStrategies.java
b/fe/fe-core/src/main/java/org/apache/doris/nereids/rules/implementation/AggregateStrategies.java
index bc6438698b8..b0dc35a4d68 100644
---
a/fe/fe-core/src/main/java/org/apache/doris/nereids/rules/implementation/AggregateStrategies.java
+++
b/fe/fe-core/src/main/java/org/apache/doris/nereids/rules/implementation/AggregateStrategies.java
@@ -738,8 +738,6 @@ public class AggregateStrategies implements
ImplementationRuleFactory {
if (column.isAggregated()) {
return canNotPush;
}
- // The zone map max length of CharFamily is 512, do not
- // over the length: https://github.com/apache/doris/pull/6293
if (mergeOp == PushDownAggOp.MIN_MAX || mergeOp ==
PushDownAggOp.MIX) {
if (logicalScan instanceof LogicalOlapScan
&& ((LogicalOlapScan) logicalScan).getTable()
instanceof RowBinlogTableWrapper
@@ -747,11 +745,7 @@ public class AggregateStrategies implements
ImplementationRuleFactory {
return canNotPush;
}
PrimitiveType colType = column.getType().getPrimitiveType();
- if (colType.isComplexType() || colType.isHllType() ||
colType.isBitmapType()
- || (colType == PrimitiveType.STRING &&
!enablePushDownStringMinMax())) {
- return canNotPush;
- }
- if (colType.isCharFamily() && column.getType().getLength() >
512 && !enablePushDownStringMinMax()) {
+ if (colType.isComplexType() || colType.isHllType() ||
colType.isBitmapType()) {
return canNotPush;
}
}
@@ -815,11 +809,6 @@ public class AggregateStrategies implements
ImplementationRuleFactory {
}
}
- private boolean enablePushDownStringMinMax() {
- ConnectContext connectContext = ConnectContext.get();
- return connectContext != null &&
connectContext.getSessionVariable().isEnablePushDownStringMinMax();
- }
-
private boolean enablePushDownNoGroupAgg() {
ConnectContext connectContext = ConnectContext.get();
return connectContext == null ||
connectContext.getSessionVariable().enablePushDownNoGroupAgg();
diff --git a/fe/fe-core/src/main/java/org/apache/doris/qe/SessionVariable.java
b/fe/fe-core/src/main/java/org/apache/doris/qe/SessionVariable.java
index 2f5fc707cfb..47571df0c61 100644
--- a/fe/fe-core/src/main/java/org/apache/doris/qe/SessionVariable.java
+++ b/fe/fe-core/src/main/java/org/apache/doris/qe/SessionVariable.java
@@ -787,7 +787,7 @@ public class SessionVariable implements Serializable,
Writable {
public static final String KEEP_CARRIAGE_RETURN = "keep_carriage_return";
- public static final String ENABLE_PUSHDOWN_STRING_MINMAX =
"enable_pushdown_string_minmax";
+ public static final String FORCE_PUSHDOWN_ZONEMAP_MINMAX =
"force_pushdown_zonemap_minmax";
public static final String ENABLE_MOR_VALUE_PREDICATE_PUSHDOWN_TABLES
= "enable_mor_value_predicate_pushdown_tables";
@@ -2370,10 +2370,13 @@ public class SessionVariable implements Serializable,
Writable {
+ "pushdown minmax on unique table.")
public boolean enablePushDownMinMaxOnUnique = false;
- // Whether enable push down string type minmax to scan node.
- @VarAttrDef.VarAttr(name = ENABLE_PUSHDOWN_STRING_MINMAX, needForward =
true, description = "Set whether to enable "
- + "push down string type minmax.")
- public boolean enablePushDownStringMinMax = false;
+ // Whether to force MIN/MAX onto the zone map when its bound is not a
value the data holds now:
+ // a cut string bound, or one covering rows a delete predicate removed.
The alias is the old
+ // name, from when this only governed string bounds.
+ @VarAttrDef.VarAttr(name = FORCE_PUSHDOWN_ZONEMAP_MINMAX, alias =
{"enable_pushdown_string_minmax"},
+ needForward = true, description = "Set whether to force a pushed
down minmax onto the zone map when its "
+ + "bound is a cut string prefix, or still covers rows a delete
predicate removed.")
+ public boolean forcePushDownZonemapMinMax = false;
// Comma-separated list of MOR tables to enable value predicate pushdown.
@VarAttrDef.VarAttr(name = ENABLE_MOR_VALUE_PREDICATE_PUSHDOWN_TABLES,
needForward = true, description = "Comma-sep"
@@ -5044,10 +5047,6 @@ public class SessionVariable implements Serializable,
Writable {
this.enablePushDownMinMaxOnUnique = enablePushDownMinMaxOnUnique;
}
- public boolean isEnablePushDownStringMinMax() {
- return enablePushDownStringMinMax;
- }
-
public String getEnableMorValuePredicatePushdownTables() {
return enableMorValuePredicatePushdownTables;
}
@@ -5526,6 +5525,7 @@ public class SessionVariable implements Serializable,
Writable {
tResult.setEnableInvertedIndexQuery(enableInvertedIndexQuery);
tResult.setEnableNoNeedReadDataOpt(enableNoNeedReadDataOpt);
+ tResult.setForcePushdownZonemapMinmax(forcePushDownZonemapMinMax);
if (dryRunQuery) {
tResult.setDryRunQuery(true);
diff --git
a/fe/fe-core/src/main/java/org/apache/doris/statistics/util/StatisticsUtil.java
b/fe/fe-core/src/main/java/org/apache/doris/statistics/util/StatisticsUtil.java
index 9b81f4fe538..ff8d7d06850 100644
---
a/fe/fe-core/src/main/java/org/apache/doris/statistics/util/StatisticsUtil.java
+++
b/fe/fe-core/src/main/java/org/apache/doris/statistics/util/StatisticsUtil.java
@@ -202,7 +202,7 @@ public class StatisticsUtil {
sessionVariable.enableFileCache = false;
sessionVariable.forbidUnknownColStats = false;
sessionVariable.enablePushDownMinMaxOnUnique = true;
- sessionVariable.enablePushDownStringMinMax = true;
+ sessionVariable.forcePushDownZonemapMinMax = true;
sessionVariable.enableUniqueKeyPartialUpdate = false;
sessionVariable.enableMaterializedViewRewrite = false;
sessionVariable.enableQueryCache = false;
diff --git a/gensrc/thrift/PaloInternalService.thrift
b/gensrc/thrift/PaloInternalService.thrift
index afbfe22b02f..3f80d4ae5a6 100644
--- a/gensrc/thrift/PaloInternalService.thrift
+++ b/gensrc/thrift/PaloInternalService.thrift
@@ -534,6 +534,12 @@ struct TQueryOptions {
// index reads -- the two formats amplify write-back differently, so each
// needs its own switch.
1005: optional bool inverted_index_snii_read_no_write_file_cache = false
+ // Whether to force a pushed-down MIN/MAX onto the zone map even when its
bound is not a value
+ // the data holds right now: a string bound cut at 512 bytes is a prefix,
and any bound still
+ // covers rows a delete predicate removed. Statistics collection sets it;
every other query
+ // reads the data instead.
+ // Defaults to false because an old FE never sends this field, and BE
checked both cases before.
+ 1006: optional bool force_pushdown_zonemap_minmax = false
}
diff --git
a/regression-test/data/query_p0/explain/test_pushdown_zonemap_minmax.out
b/regression-test/data/query_p0/explain/test_pushdown_zonemap_minmax.out
new file mode 100644
index 00000000000..1c9976143ee
--- /dev/null
+++ b/regression-test/data/query_p0/explain/test_pushdown_zonemap_minmax.out
@@ -0,0 +1,19 @@
+-- This file is automatically generated. You should know what you did if you
want to edit this
+-- !wide_short --
+aaa zzz
+
+-- !wide_long --
+aaa 600 zzzz
+
+-- !512_max --
+512 dddd
+
+-- !512_eq --
+1
+
+-- !str_off --
+aaa 600 zzzz
+
+-- !str_on --
+512 zzz{
+
diff --git
a/regression-test/suites/query_p0/explain/test_pushdown_zonemap_minmax.groovy
b/regression-test/suites/query_p0/explain/test_pushdown_zonemap_minmax.groovy
new file mode 100644
index 00000000000..c10d180ef99
--- /dev/null
+++
b/regression-test/suites/query_p0/explain/test_pushdown_zonemap_minmax.groovy
@@ -0,0 +1,102 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+// MIN/MAX push-down no longer looks at the declared column length. The
storage layer decides per
+// segment: a zone map bound cut to 512 bytes is not a value the column holds,
so those segments
+// read the rows instead.
+suite("test_pushdown_zonemap_minmax") {
+ def longValue = "z" * 600
+ def exactValue = "d" * 512
+
+ // A VARCHAR wider than 512 used to be excluded by its declared length
alone, even when every
+ // value in it was short.
+ sql "DROP TABLE IF EXISTS test_string_minmax_wide"
+ sql """
+ CREATE TABLE test_string_minmax_wide (
+ `id` INT NOT NULL,
+ `v` VARCHAR(65533) NOT NULL
+ ) ENGINE=OLAP
+ DUPLICATE KEY(`id`)
+ DISTRIBUTED BY HASH(`id`) BUCKETS 1
+ PROPERTIES ("replication_allocation" = "tag.location.default: 1");
+ """
+ sql """ INSERT INTO test_string_minmax_wide VALUES (1, "aaa"), (2, "zzz")
"""
+ explain {
+ sql "select min(v), max(v) from test_string_minmax_wide"
+ contains "pushAggOp=MINMAX"
+ }
+ qt_wide_short "select min(v), max(v) from test_string_minmax_wide"
+
+ // A value past the 512-byte cut: the plan still pushes down, and the
storage layer falls back
+ // per segment so the answer stays a value the table holds.
+ sql """ INSERT INTO test_string_minmax_wide VALUES (3, "${longValue}") """
+ explain {
+ sql "select min(v), max(v) from test_string_minmax_wide"
+ contains "pushAggOp=MINMAX"
+ }
+ qt_wide_long "select min(v), length(max(v)), right(max(v), 4) from
test_string_minmax_wide"
+
+ // A VARCHAR filled to exactly 512 bytes is cut as well. FE pushed this
down before too,
+ // because the declared length is not over 512, and the raised bound
answered MAX with a value
+ // that was never inserted.
+ sql "DROP TABLE IF EXISTS test_string_minmax_512"
+ sql """
+ CREATE TABLE test_string_minmax_512 (
+ `id` INT NOT NULL,
+ `v` VARCHAR(512) NOT NULL
+ ) ENGINE=OLAP
+ DUPLICATE KEY(`id`)
+ DISTRIBUTED BY HASH(`id`) BUCKETS 1
+ PROPERTIES ("replication_allocation" = "tag.location.default: 1");
+ """
+ sql """ INSERT INTO test_string_minmax_512 VALUES (1, "aaa"), (2,
"${exactValue}") """
+ qt_512_max "select length(max(v)), right(max(v), 4) from
test_string_minmax_512"
+ qt_512_eq "select count(*) from test_string_minmax_512 where v =
'${exactValue}'"
+
+ // The switch says whether a cut bound may answer MIN/MAX. It is off by
default, so the answer
+ // is always a value the table holds. Statistics collection turns it on
and takes the cut bound
+ // rather than reading every row.
+ sql "DROP TABLE IF EXISTS test_string_minmax_str"
+ sql """
+ CREATE TABLE test_string_minmax_str (
+ `id` INT NOT NULL,
+ `v` STRING NOT NULL
+ ) ENGINE=OLAP
+ DUPLICATE KEY(`id`)
+ DISTRIBUTED BY HASH(`id`) BUCKETS 1
+ PROPERTIES ("replication_allocation" = "tag.location.default: 1");
+ """
+ sql """ INSERT INTO test_string_minmax_str VALUES (1, "aaa"), (2,
"${longValue}") """
+
+ // Off by default: the plan still pushes down, and the segment whose bound
was cut reads its
+ // rows, so both answers are values the table holds.
+ explain {
+ sql "select min(v), max(v) from test_string_minmax_str"
+ contains "pushAggOp=MINMAX"
+ }
+ qt_str_off "select min(v), length(max(v)), right(max(v), 4) from
test_string_minmax_str"
+
+ // On: the cut bound answers straight away. It is the 512-byte prefix with
its last byte
+ // raised, so the max ends in '{', one past the 'z' that was inserted.
+ sql "set force_pushdown_zonemap_minmax = true"
+ explain {
+ sql "select min(v), max(v) from test_string_minmax_str"
+ contains "pushAggOp=MINMAX"
+ }
+ qt_str_on "select length(max(v)), right(max(v), 4) from
test_string_minmax_str"
+ sql "set force_pushdown_zonemap_minmax = false"
+}
diff --git a/regression-test/suites/statistics/analyze_stats.groovy
b/regression-test/suites/statistics/analyze_stats.groovy
index 7814f2924d1..c0eaff54e2b 100644
--- a/regression-test/suites/statistics/analyze_stats.groovy
+++ b/regression-test/suites/statistics/analyze_stats.groovy
@@ -2747,11 +2747,8 @@ PARTITION `p599` VALUES IN (599)
"""
sql """insert into string_min_max values (1,'name1'), (2, 'name2')"""
sql """analyze table string_min_max with sync"""
- explain {
- sql("select min(name), max(name) from string_min_max")
- contains "pushAggOp=NONE"
- }
- sql """set enable_pushdown_string_minmax = true"""
+ // Every string column is pushed down now, and the storage layer decides
per segment whether
+ // a cut bound may answer. These bounds are short, so the zone map answers
them.
explain {
sql("select min(name), max(name) from string_min_max")
contains "pushAggOp=MINMAX"
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
