This is an automated email from the ASF dual-hosted git repository.
yiguolei pushed a commit to branch branch-4.2
in repository https://gitbox.apache.org/repos/asf/doris.git
The following commit(s) were added to refs/heads/branch-4.2 by this push:
new 031bb541b08 [branch-4.2][fix](zonemap) Do not trust a cut string bound
to prune or to answer MIN/MAX (#67642) (#68324)
031bb541b08 is described below
commit 031bb541b084e45cfc9f8d44145fe0b779ed2e49
Author: Selectdb Robot <[email protected]>
AuthorDate: Tue Sep 22 10:26:27 2026 +0800
[branch-4.2][fix](zonemap) Do not trust a cut string bound to prune or to
answer MIN/MAX (#67642) (#68324)
### What problem does this PR solve?
Related PR: #67642
Backport #67642 to `branch-4.2`, cherry-picked from
`b0f266a8ebc1a333f806831f5d2209f837fda5d5`.
Fix string ZoneMap bounds at the 512-byte cutoff: carry when raising the
maximum, discard an all-zero overflowed range on read, and fall back to
reading rows for exact MIN/MAX when bounds were truncated. Preserve the
original statistics-collection override and the old SQL variable name as
an alias.
### Branch compatibility adaptations
- `segment.cpp`: retain branch-4.2's `Schema` signature and column-ID
iteration; use `schema.column(schema.column_id(i))` for the new
string-type check. Add the original MIN/MAX forcing and fallback logic.
- `SessionVariable.java`: use branch-4.2's `VariableMgr.VarAttr` with
Chinese/English descriptions, preserving the new variable name, old
alias, default, and Thrift mapping.
- `PaloInternalService.thrift`: add only `force_pushdown_zonemap_minmax`
with the original field ID `1006` and default `false`. Do not import the
unrelated `1005` SNII option from master conflict context.
- `vgeneric_iterators_test.cpp`: although this file auto-merged, adapt
the added fixture to branch-4.2's `Schema`/`SchemaSPtr`,
`SegmentWriterOptions`, `TestSegmentWriter`, and `finalize()` APIs. All
original test cases remain.
### Validation
- Verified all 12 original files are included; added/deleted lines in
the other 9 files match the original patch.
- No unresolved conflicts, conflict markers, or unstaged edits. Code
whitespace check passed. The generated `.out` retains the original final
blank separator, which default `git diff --check` reports as a blank
line at EOF.
- Confirmed the target Schema/writer/annotation APIs and checked that
`TQueryOptions` field IDs are unique.
- Regression suite and expected output retain the original content; all
added BE test cases are retained.
- Compilation, BE unit tests, and runtime regression tests were not run
in this session; CI validation remains pending.
### Release note
Fix incorrect string ZoneMap pruning and MIN/MAX results for truncated
string bounds.
### Check List (For Author)
- Test
- [x] Regression tests included from #67642 (not run in this session)
- [x] Unit tests included from #67642 with branch API adaptations (not
run in this session)
- [ ] Manual test
- Behavior changed:
- [x] Yes. Apply the ZoneMap and MIN/MAX correctness fixes from #67642.
- Does this need documentation?
- [x] No additional documentation changes for this backport.
### Check List (For Reviewer who merge this PR)
- [ ] Confirm the release note
- [ ] Confirm test cases
- [ ] Confirm document
- [ ] Add branch pick label
Co-authored-by: Chenyang Sun <[email protected]>
---
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 | 200 +++++++++++++++++++++
be/test/storage/segment/zone_map_index_test.cpp | 129 +++++++++++++
.../rules/implementation/AggregateStrategies.java | 13 +-
.../java/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 ++
.../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 2ee6f828d1e..cc5f73ee0ab 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<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 7e2ee007f47..f16004472c0 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 c293434abee..0913e80f364 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 cd43d149db4..7ee626167b6 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<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());
+ 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<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(&file_size, &index_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<Schema>(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<SchemaSPtr> _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<Schema>(schema);
diff --git a/be/test/storage/segment/zone_map_index_test.cpp
b/be/test/storage/segment/zone_map_index_test.cpp
index 36a4883f7c1..9f46a5821c3 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<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 9d4814aea80..da0193aa0c5 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 @@ 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) {
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 @@ 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 1bf84014bbc..0c099760736 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 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";
@@ -2443,10 +2443,15 @@ public class SessionVariable implements Serializable,
Writable {
"是否启用 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 class SessionVariable implements Serializable,
Writable {
this.enablePushDownMinMaxOnUnique = enablePushDownMinMaxOnUnique;
}
- public boolean isEnablePushDownStringMinMax() {
- return enablePushDownStringMinMax;
- }
-
public String getEnableMorValuePredicatePushdownTables() {
return enableMorValuePredicatePushdownTables;
}
@@ -5819,6 +5820,7 @@ public class SessionVariable implements Serializable,
Writable {
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 0bc6ea8a699..3235f6c7fba 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 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 9e6d34c7c39..e2ba8937684 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 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]
