From bdc1c716039a89f341dc711737bf32c428426a02 Mon Sep 17 00:00:00 2001 From: Chenyang Sun Date: Thu, 17 Sep 2026 21:31:50 +0800 Subject: [PATCH] [fix](zonemap) Do not trust a cut string bound to prune or to answer MIN/MAX (#67642) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. (cherry picked from commit b0f266a8ebc1a333f806831f5d2209f837fda5d5) Adapt Schema and test writer APIs plus session-variable annotations to branch-4.2. Keep Thrift field ID 1006 without importing the unrelated master field 1005. --- .../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 | 200 ++++++++++++++++++ .../storage/segment/zone_map_index_test.cpp | 129 +++++++++++ .../implementation/AggregateStrategies.java | 13 +- .../org/apache/doris/qe/SessionVariable.java | 20 +- .../doris/statistics/util/StatisticsUtil.java | 2 +- gensrc/thrift/PaloInternalService.thrift | 6 + .../explain/test_pushdown_zonemap_minmax.out | 19 ++ .../test_pushdown_zonemap_minmax.groovy | 102 +++++++++ .../suites/statistics/analyze_stats.groovy | 7 +- 12 files changed, 523 insertions(+), 43 deletions(-) create mode 100644 regression-test/data/query_p0/explain/test_pushdown_zonemap_minmax.out create mode 100644 regression-test/suites/query_p0/explain/test_pushdown_zonemap_minmax.groovy 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 2ee6f828d1e934..cc5f73ee0ab5f8 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" @@ -100,6 +101,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. @@ -247,11 +256,18 @@ void TypedZoneMapIndexWriter::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(); 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(str[i - 1]) + 1; + str[i - 1] = static_cast(byte); + if (static_cast(byte) != 0) { + break; + } + } } } } diff --git a/be/src/storage/segment/column_reader.cpp b/be/src/storage/segment/column_reader.cpp index 7e2ee007f47474..f16004472c0e9a 100644 --- a/be/src/storage/segment/column_reader.cpp +++ b/be/src/storage/segment/column_reader.cpp @@ -450,9 +450,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 c293434abee129..0913e80f364114 100644 --- a/be/src/storage/segment/segment.cpp +++ b/be/src/storage/segment/segment.cpp @@ -148,8 +148,14 @@ 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. +// 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 Schema& schema, const StorageReadOptions& read_options, bool* usable) { *usable = true; @@ -168,10 +174,26 @@ Status segment_zone_maps_can_answer_agg(Segment* segment, const Schema& schema, } 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(schema.column_id(i))->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(); } @@ -463,16 +485,17 @@ Status Segment::new_iterator(SchemaSPtr schema, const StorageReadOptions& read_o 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 cd43d149db41d2..7ee626167b6d18 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" @@ -186,6 +191,201 @@ 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(); + 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 pushdown_iterator_for( + const std::string& name, const std::vector& 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()); + SegmentWriterOptions writer_options; + writer_options.num_rows_per_block = 1024; + TestSegmentWriter 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(static_cast(i)); + row.mutable_field(1) = Field::create_field(String(values[i])); + EXPECT_TRUE(writer.append_row(row).ok()); + } + uint64_t file_size = 0; + uint64_t index_size = 0; + EXPECT_TRUE(writer.finalize(&file_size, &index_size).ok()); + EXPECT_TRUE(file_writer->close().ok()); + + std::shared_ptr segment; + EXPECT_TRUE(segment_v2::Segment::open(_fs, segment_path, 100, 0, RowsetId {.version = 1}, + tablet_schema, io::FileReaderOptions {}, &segment) + .ok()); + + std::vector column_ids {0, 1}; + // VStatisticsIterator keeps a reference to the schema, so it has to outlive the iterator. + auto schema = std::make_shared(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(); + 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 iter; + EXPECT_TRUE(segment->new_iterator(schema, read_options, &iter).ok()); + return iter; + } + + std::shared_ptr _fs; + OlapReaderStatistics _stats; + std::vector> _states; + std::vector _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(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(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(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(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(0xff)); + auto iter = pushdown_iterator_for("pass_all_approx", {"aaa", wrapping}, + /*accept_cut_bound=*/true); + EXPECT_NE(dynamic_cast(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(0xff)); + auto iter = pushdown_iterator_for("pass_all_exact", {"aaa", wrapping}); + EXPECT_EQ(dynamic_cast(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(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(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(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(schema); diff --git a/be/test/storage/segment/zone_map_index_test.cpp b/be/test/storage/segment/zone_map_index_test.cpp index 36a4883f7c12ef..9f46a5821c34c4 100644 --- a/be/test/storage/segment/zone_map_index_test.cpp +++ b/be/test/storage/segment/zone_map_index_test.cpp @@ -1598,5 +1598,134 @@ TEST_F(ColumnZoneMapTest, EmbeddedNulKeepsStringBound) { test_embedded_nul_bound("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 writer; + EXPECT_TRUE(ZoneMapIndexWriter::create(data_type, tab_col.get(), writer).ok()); + segment_v2::ZoneMap zone_map; + zone_map.min_value = Field::create_field(value); + zone_map.max_value = Field::create_field(value); + zone_map.has_not_null = true; + writer->modify_index_before_flush(zone_map); + return Raised {zone_map.max_value.get(), 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(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(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(0xe4)); // first byte of a three-byte character + cut_mid_char.push_back(static_cast(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(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(0xe4)); + cut_on_boundary.push_back(static_cast(0xb8)); + cut_on_boundary.push_back(static_cast(0xad)); + const auto whole_raised = raise_max(cut_on_boundary); + EXPECT_FALSE(whole_raised.pass_all); + EXPECT_EQ(static_cast(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(0xe4)); + whole_raised.push_back(static_cast(0xb8)); + whole_raised.push_back(static_cast(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(0xe4)); + cut_raised.push_back(static_cast(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(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(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 9d4814aea801d8..da0193aa0c5d30 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,15 +738,9 @@ private LogicalAggregate storageLayerAggregate( 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) { 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; } } @@ -811,11 +805,6 @@ private LogicalAggregate storageLayerAggregate( } } - 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 1bf84014bbc984..0c099760736e5c 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 @@ -801,7 +801,7 @@ public String toString() { 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"; @@ -2443,10 +2443,15 @@ public boolean isEnableHboNonStrictMatchingMode() { "是否启用 pushdown minmax on unique table。", "Set whether to pushdown minmax on unique table."}) public boolean enablePushDownMinMaxOnUnique = false; - // Whether enable push down string type minmax to scan node. - @VariableMgr.VarAttr(name = ENABLE_PUSHDOWN_STRING_MINMAX, needForward = true, description = { - "是否启用 string 类型 min max 下推。", "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. + @VariableMgr.VarAttr(name = FORCE_PUSHDOWN_ZONEMAP_MINMAX, alias = {"enable_pushdown_string_minmax"}, + needForward = true, description = { + "当 ZoneMap 边界为截断的字符串前缀或仍覆盖已删除行时，是否强制使用 ZoneMap 执行下推的 MIN/MAX。", + "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. @VariableMgr.VarAttr(name = ENABLE_MOR_VALUE_PREDICATE_PUSHDOWN_TABLES, needForward = true, description = { @@ -5314,10 +5319,6 @@ public void setEnablePushDownMinMaxOnUnique(boolean enablePushDownMinMaxOnUnique this.enablePushDownMinMaxOnUnique = enablePushDownMinMaxOnUnique; } - public boolean isEnablePushDownStringMinMax() { - return enablePushDownStringMinMax; - } - public String getEnableMorValuePredicatePushdownTables() { return enableMorValuePredicatePushdownTables; } @@ -5819,6 +5820,7 @@ public TQueryOptions toThrift() { tResult.setEnableInvertedIndexQuery(enableInvertedIndexQuery); tResult.setEnableCommonExprPushdownForInvertedIndex(enableCommonExpPushDownForInvertedIndex); 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 0bc6ea8a6992ad..3235f6c7fbae2e 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 @@ -228,7 +228,7 @@ public static AutoCloseConnectContext buildConnectContext(boolean useFileCacheFo 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 9e6d34c7c39621..e2ba8937684d14 100644 --- a/gensrc/thrift/PaloInternalService.thrift +++ b/gensrc/thrift/PaloInternalService.thrift @@ -519,6 +519,12 @@ struct TQueryOptions { 1002: optional bool enable_file_scanner_v2 = false 1003: optional bool enable_topn_lazy_mat_phase2_no_write_file_cache = false 1004: optional i64 file_cache_query_limit_bytes = -1 + // 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 00000000000000..1c9976143ee4f6 --- /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 00000000000000..c10d180ef994cc --- /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 7814f2924d106b..c0eaff54e2babf 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"