github-actions[bot] commented on code in PR #67642:
URL: https://github.com/apache/doris/pull/67642#discussion_r4003745689
##########
be/src/storage/segment/segment.cpp:
##########
@@ -169,10 +175,26 @@ Status segment_zone_maps_can_answer_agg(Segment* segment,
const ReadSchema& sche
}
ZoneMap zone_map;
RETURN_IF_ERROR(reader->get_segment_zone_map(&zone_map));
+
+ // The zone map gave up its range, so it has no min/max left to answer
with.
if (zone_map.pass_all) {
*usable = false;
return Status::OK();
}
+
+ // Only a string bound is cut at MAX_ZONE_MAP_INDEX_SIZE, and a column
of nothing but
+ // nulls stored no bound to look at.
+ if (!is_string_type(schema.column(ordinal)->type()) ||
!zone_map.has_not_null) {
+ continue;
+ }
+
+ // A cut bound is not a value the column holds: the min is a prefix of
the smallest value
+ // and the max was raised past the largest one. Neither can answer
MIN()/MAX().
+ if (zone_map.min_value.as_string_view().size() >=
MAX_ZONE_MAP_INDEX_SIZE ||
+ zone_map.max_value.as_string_view().size() >=
MAX_ZONE_MAP_INDEX_SIZE) {
Review Comment:
[P1] Reject lossy embedded-NUL STRING bounds
This check sees an already-deserialized Field. STRING writes preserve the
full `ColumnString` byte length and zone maps serialize those raw bytes, but
`from_olap_string` uses `strnlen` for all string types. Thus a short five-byte
STRING containing an embedded NUL is shortened at that byte, remains
`pass_all=false`, passes the `< 512` checks here, and is returned by
`VStatisticsIterator`. Since this PR removes the default FE gate for
`PrimitiveType.STRING`, exact user MIN/MAX now reaches that wrong-result path
without opting in. Please preserve raw bytes for STRING/VARCHAR (trimming only
CHAR padding), or reject such lossy bounds before selecting the statistics
iterator, with an end-to-end embedded-NUL test.
##########
be/src/storage/segment/segment.cpp:
##########
@@ -503,16 +525,17 @@ Status Segment::new_iterator(ReadSchemaSPtr schema, const
StorageReadOptions& re
RETURN_IF_ERROR(load_index(read_options.stats, &read_options.io_ctx));
}
+ // COUNT and MIX report the segment row count, which a delete predicate
makes wrong whatever
+ // the zone map bounds hold, so they keep the guard below even when the
switch is on.
+ const auto agg = read_options.push_down_agg_type_opt;
+ const bool forced = pushdown_zonemap_minmax_forced(read_options);
bool use_statistics_iterator =
-
read_options.delete_condition_predicates->num_of_column_predicate() == 0 &&
- read_options.push_down_agg_type_opt != TPushAggOp::NONE &&
- read_options.push_down_agg_type_opt != TPushAggOp::COUNT_ON_INDEX;
- // COUNT only fills defaults, every other pushed-down aggregate reads
min/max out of the
- // segment zone maps.
- if (use_statistics_iterator && read_options.push_down_agg_type_opt !=
TPushAggOp::COUNT) {
- bool usable = false;
- RETURN_IF_ERROR(segment_zone_maps_can_answer_agg(this, *schema,
read_options, &usable));
- use_statistics_iterator = usable;
+ agg != TPushAggOp::NONE && agg != TPushAggOp::COUNT_ON_INDEX &&
+ (forced ||
read_options.delete_condition_predicates->num_of_column_predicate() == 0);
+ // COUNT only fills defaults, every other aggregate reads min/max out of
the zone maps.
+ if (use_statistics_iterator && !forced && agg != TPushAggOp::COUNT) {
+ RETURN_IF_ERROR(segment_zone_maps_can_answer_agg(this, *schema,
read_options,
Review Comment:
[P1] Do not force unreadable `pass_all` bounds into MIN/MAX
`forced` skips the validity helper for every `pass_all` reason, including
bounds that are not safe approximations. For the new test's 512-byte value
ending in `0xff`, the writer stores a max ending in NUL; `from_olap_string`
applies `strnlen` before `from_proto` sets `pass_all`, so the emitted max is
only the preceding 511-byte prefix—strictly below the real value. The test
checks only `next_batch().ok()`, not that value. Please distinguish
deliberately inexact-but-materialized cut/delete bounds from wrapped or
unparsable maps, and keep the latter on the row-scan fallback.
##########
be/src/storage/index/zone_map/zone_map_index.cpp:
##########
@@ -85,6 +87,17 @@ Status ZoneMap::from_proto(const ZoneMapPB& zone_map, const
DataTypePtr& data_ty
parse_bound(zone_map.max(), zone_map_info.max_value);
}
+ // Lower the raised byte back to what the data held, then run the
writer's check on it.
+ // A max that came from 0xff wrapped to 0x00, and old segments still
carry such a max.
+ if (!zone_map_info.pass_all && is_string_type(field_type) &&
+ zone_map.max().size() == MAX_ZONE_MAP_INDEX_SIZE) {
+ std::string max_before_raise = zone_map.max();
+ max_before_raise.back() -= 1;
+ if (!validate_utf8(max_before_raise.data(),
max_before_raise.size())) {
+ zone_map_info.pass_all = true;
Review Comment:
[P2] Detect byte wrap without rejecting split UTF-8 prefixes
Invalid UTF-8 after decrementing the last byte is not specific to the `0xff`
wrap. A valid long value whose 512-byte cut ends midway through a multibyte
character reconstructs to invalid UTF-8, while the stored non-wrapping raised
byte is still a sound upper bound under Doris's bytewise comparison. Marking it
`pass_all` makes segment and page pruning retain every such zone (and the added
test explicitly pins this safe split as unusable). Please detect the actual
wrap—for example, the raised stored byte becoming `0x00`, or explicit writer
metadata—and add a pruning test for a split multibyte prefix.
##########
fe/fe-core/src/main/java/org/apache/doris/qe/SessionVariable.java:
##########
@@ -2380,10 +2380,13 @@ public boolean isEnableHboNonStrictMatchingMode() {
+ "pushdown minmax on unique table.")
public boolean enablePushDownMinMaxOnUnique = false;
- // Whether enable push down string type minmax to scan node.
- @VarAttrDef.VarAttr(name = ENABLE_PUSHDOWN_STRING_MINMAX, needForward =
true, description = "Set whether to enable "
- + "push down string type minmax.")
- public boolean enablePushDownStringMinMax = false;
+ // Whether to force MIN/MAX onto the zone map when its bound is not a
value the data holds now:
+ // a cut string bound, or one covering rows a delete predicate removed.
The alias is the old
+ // name, from when this only governed string bounds.
+ @VarAttrDef.VarAttr(name = FORCE_PUSHDOWN_ZONEMAP_MINMAX, alias =
{"enable_pushdown_string_minmax"},
+ needForward = true, description = "Set whether to force a pushed
down minmax onto the zone map when its "
Review Comment:
[P1] Include the expanded force flag in result-cache identity
This flag now changes execution results for numeric and short-string MIN/MAX
too: with a delete predicate, `true` reads zone-map extrema that may belong to
deleted rows, while `false` falls back to live rows. Yet the annotation omits
`affectQueryResultInExecution`, so both SQL-cache variable checks and the
fragment query-cache digest ignore it. A result cached with force enabled can
therefore be replayed after the session disables forcing, despite the table
version and plan being unchanged. Please mark this as
execution-result-affecting and cover cache population under one value followed
by lookup under the other.
##########
fe/fe-core/src/main/java/org/apache/doris/qe/SessionVariable.java:
##########
@@ -794,7 +794,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";
Review Comment:
[P2] Preserve the old name for persistence and forwarding
Making `enable_pushdown_string_minmax` only an alias does not migrate stored
or forwarded state. `SessionVariable.toJson`/`readFromJson` and the
forwarded-variable map use only `VarAttr.name()`; aliases are registered only
for SQL lookup. Consequently, an FE image containing an old global value of
`true` reloads this renamed field as its default `false`, and old/new FEs
ignore each other's forwarding key. Please retain the old name as the
persisted/forwarded primary (with the new spelling as an alias), or make both
replay paths alias-aware and cover old-image and mixed-FE cases.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
