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"