github-actions[bot] commented on code in PR #67019:
URL: https://github.com/apache/doris/pull/67019#discussion_r3827778975
##########
be/src/storage/predicate/comparison_predicate.h:
##########
@@ -130,26 +130,55 @@ class ComparisonPredicateBase final : public
ColumnPredicate {
_evaluate_bit<true>(column, sel, size, flags);
}
- bool evaluate_and(const segment_v2::ZoneMap& zone_map) const override {
+ ZoneMapFilterResult evaluate_zonemap_filter_impl(
+ const segment_v2::ZoneMap& zone_map) const override {
+ // Every row is NULL, and a comparison against NULL never passes the
filter.
if (!zone_map.has_not_null) {
- return false;
+ return ZoneMapFilterResult::kNoMatch;
}
+ const auto& min_value = zone_map.min_value.template get<Type>();
+ const auto& max_value = zone_map.max_value.template get<Type>();
+ // One NULL row is enough to stop the whole zone from matching.
+ const bool can_match_all = !zone_map.has_null;
+ // EQ and NE read the same two facts and reach opposite conclusions.
if constexpr (PT == PredicateType::EQ) {
- return _operator(
- Compare::less_equal(zone_map.min_value.template
get<Type>(), _value) &&
- Compare::greater_equal(zone_map.max_value.template
get<Type>(), _value),
- true);
+ // The value sits outside [min, max], so no row can be equal to it.
+ if (Compare::less(_value, min_value) || Compare::less(max_value,
_value)) {
+ return ZoneMapFilterResult::kNoMatch;
+ }
+ // The zone holds nothing but the value.
+ if (can_match_all && Compare::equal(min_value, _value) &&
+ Compare::equal(max_value, _value)) {
+ return ZoneMapFilterResult::kAllMatch;
+ }
+ return ZoneMapFilterResult::kMayMatch;
} else if constexpr (PT == PredicateType::NE) {
- return _operator(
- Compare::equal(zone_map.min_value.template get<Type>(),
_value) &&
- Compare::equal(zone_map.max_value.template
get<Type>(), _value),
- true);
- } else if constexpr (PT == PredicateType::LT || PT ==
PredicateType::LE) {
- return _operator(zone_map.min_value.template get<Type>(), _value);
+ // The zone holds nothing but the value, so no row can differ from
it.
+ if (Compare::equal(min_value, _value) && Compare::equal(max_value,
_value)) {
+ return ZoneMapFilterResult::kNoMatch;
+ }
+ // The value sits outside [min, max], so no row can be equal to it.
+ if (can_match_all &&
+ (Compare::less(_value, min_value) || Compare::less(max_value,
_value))) {
+ return ZoneMapFilterResult::kAllMatch;
Review Comment:
This new `kAllMatch` case is unsafe for truncated string zone maps. Doris
VARCHAR/STRING can store invalid UTF-8 (the existing `is_valid_utf8` regression
inserts `unhex('FF')`), while `modify_index_before_flush` increments byte 511
of every 512-byte maximum. For `L = repeat('a', 511) || 0xff`, that increment
wraps the serialized max to the same prefix ending in `0x00`, so a one-row
segment has `min=L` and `max<L`. `WHERE v != L` then takes `max < _value`,
reports all-match, and drops the residual, returning the row instead of zero;
the new NOT IN branch has the same problem. The same inexact bound also
invalidates the new EQ/IN/`starts_with` all-match proofs on multi-value zones.
Please carry the successor across trailing `0xff` bytes or track bound
exactness and forbid all-match proofs from inexact string bounds, with
regressions for these operators.
##########
be/src/exprs/vcompound_pred.h:
##########
@@ -142,7 +142,7 @@ class VCompoundPred : public VectorizedFnCall {
return child->can_evaluate_zonemap_filter();
});
case TExprOpcode::COMPOUND_OR:
- return !_children.empty() && std::ranges::all_of(_children,
[](const VExprSPtr& child) {
+ return std::ranges::any_of(_children, [](const VExprSPtr& child) {
Review Comment:
Please keep partially supported ORs from being considered safe metadata
filters here. With an unsupported/error-sensitive branch, a supported sibling
can return `kAllMatch`, after which `Segment::new_iterator` erases the whole
residual. For example, `assert_true(k > 0, 'boom') OR v < 100` on a segment
with `v=[1,2]` normally raises from the first child for `k=0`, but this path
drops the expression and returns the row. It also makes the Parquet readers
fetch page indexes for partial ORs that can never return `kNoMatch`, since the
unsupported child holds the result at `kMayMatch`. Please require every OR
branch to support the proof, or separately represent `can disprove` versus
`safe to drop` and retain error-sensitive residuals.
--
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]
