github-actions[bot] commented on code in PR #67774:
URL: https://github.com/apache/doris/pull/67774#discussion_r3978390168
##########
be/src/exprs/function/functions_comparison.h:
##########
@@ -377,6 +436,24 @@ inline bool can_evaluate(const VExprSPtrs& arguments) {
return true;
}
+// Accept a comparison whose both operands are slot references. Kept separate
from can_evaluate on
+// purpose: can_evaluate also gates dictionary filtering and
can_evaluate_equality, and both of
+// those dereference extract_slot_and_literal behind a DORIS_CHECK, so
widening it would abort on a
+// slot-vs-slot expression. Only can_evaluate_zonemap_filter ORs this in.
+inline bool can_evaluate_slot_slot(const VExprSPtrs& arguments) {
+ auto slot_slot = expr_zonemap::extract_slot_and_slot(arguments);
+ if (!slot_slot.has_value()) {
+ return false;
+ }
+ DORIS_CHECK(slot_slot->left_type != nullptr);
+ DORIS_CHECK(slot_slot->right_type != nullptr);
+ // The two zone maps' Fields are compared directly and Field comparison
throws on mismatched
Review Comment:
**Correctness: keep v1 decimal bounds in the table comparison domain.**
The optimizer only knows that the two table slots have compatible types; it
does not guarantee that their v1 Parquet statistics share those types.
`_type_matches` accepts a file decimal when its primitive type matches, while
the Row Group path decodes each bound in the file scale and records
`slot->type()` in the context. For example, with both table slots at scale 2,
file values `a DECIMAL(...,3)=1.000` and `b DECIMAL(...,2)=2.00` are converted
to `1.00 < 2.00` for row evaluation, but their statistic `Field`s compare
unscaled payloads `1000 >= 200`, so the new LT rule prunes the matching Row
Group. Same-scale precision narrowing can likewise be pruned before
`DecimalToDecimalConverter` reports an out-of-range value. Please require exact
file/expression type equality for v1 slot-slot pruning, or convert both bounds
to the common expression domain and propagate conversion failures.
##########
be/test/exprs/expr_zonemap_filter_test.cpp:
##########
@@ -1952,4 +1969,272 @@ TEST(ExprZonemapFilterTest,
ExprContextZonemapEvaluationShortCircuitsOnNoMatch)
EXPECT_EQ(0, ctx.stats.unusable_zonemap_eval_count);
}
+TEST(ExprZonemapFilterTest,
SlotSlotNotEqualsPrunesOnlyWhenBothCollapseToOneValue) {
+ auto type = int_type();
+ auto left = make_slot(0, type);
+ auto right = make_slot(1, type);
+ FunctionComparison<NotEqualsOp, NameNotEquals> not_equals;
+
+ // Both columns hold the single value 5, so `a != b` is false on every row.
+ auto same_point =
+ make_two_slot_context(make_int_zonemap(5, 5), make_int_zonemap(5,
5), type, type);
+ EXPECT_EQ(ZoneMapFilterResult::kNoMatch,
+ not_equals.evaluate_zonemap_filter(same_point, {left, right}));
+
+ // Two different single values: `a != b` is true everywhere, so nothing
can be pruned.
+ auto other_point =
+ make_two_slot_context(make_int_zonemap(5, 5), make_int_zonemap(6,
6), type, type);
+ EXPECT_EQ(ZoneMapFilterResult::kMayMatch,
+ not_equals.evaluate_zonemap_filter(other_point, {left, right}));
+
+ // A non-degenerate range on either side leaves the outcome unknown.
+ auto left_range =
+ make_two_slot_context(make_int_zonemap(5, 6), make_int_zonemap(5,
5), type, type);
+ EXPECT_EQ(ZoneMapFilterResult::kMayMatch,
+ not_equals.evaluate_zonemap_filter(left_range, {left, right}));
+ auto right_range =
+ make_two_slot_context(make_int_zonemap(5, 5), make_int_zonemap(5,
6), type, type);
+ EXPECT_EQ(ZoneMapFilterResult::kMayMatch,
+ not_equals.evaluate_zonemap_filter(right_range, {left, right}));
+}
+
+TEST(ExprZonemapFilterTest, SlotSlotCoversAllSixOperatorsAtTheBoundary) {
+ auto type = int_type();
+ auto left = make_slot(0, type);
+ auto right = make_slot(1, type);
+ // Left entirely above right, ranges touching at 10: [10, 20] vs [1, 10].
+ auto ctx = make_two_slot_context(make_int_zonemap(10, 20),
make_int_zonemap(1, 10), type, type);
+
+ // a < b needs min_a >= max_b, which holds at the touching bound.
+ FunctionComparison<LessOp, NameLess> less;
+ EXPECT_EQ(ZoneMapFilterResult::kNoMatch, less.evaluate_zonemap_filter(ctx,
{left, right}));
+ // a <= b needs min_a > max_b, which does not hold: a == b == 10 is
possible.
+ FunctionComparison<LessOrEqualsOp, NameLessOrEquals> less_equals;
+ EXPECT_EQ(ZoneMapFilterResult::kMayMatch,
+ less_equals.evaluate_zonemap_filter(ctx, {left, right}));
+ FunctionComparison<GreaterOp, NameGreater> greater;
+ EXPECT_EQ(ZoneMapFilterResult::kMayMatch,
greater.evaluate_zonemap_filter(ctx, {left, right}));
+ FunctionComparison<EqualsOp, NameEquals> equals;
+ EXPECT_EQ(ZoneMapFilterResult::kMayMatch,
equals.evaluate_zonemap_filter(ctx, {left, right}));
+
+ // Fully disjoint: equality becomes impossible and so does a <= b.
+ auto disjoint =
+ make_two_slot_context(make_int_zonemap(10, 20),
make_int_zonemap(1, 9), type, type);
+ EXPECT_EQ(ZoneMapFilterResult::kNoMatch,
+ equals.evaluate_zonemap_filter(disjoint, {left, right}));
+ EXPECT_EQ(ZoneMapFilterResult::kNoMatch,
+ less_equals.evaluate_zonemap_filter(disjoint, {left, right}));
+
+ // Mirrored, so a > b and a >= b become impossible.
+ auto mirrored =
+ make_two_slot_context(make_int_zonemap(1, 9), make_int_zonemap(10,
20), type, type);
+ EXPECT_EQ(ZoneMapFilterResult::kNoMatch,
+ greater.evaluate_zonemap_filter(mirrored, {left, right}));
+ FunctionComparison<GreaterOrEqualsOp, NameGreaterOrEquals> greater_equals;
+ EXPECT_EQ(ZoneMapFilterResult::kNoMatch,
+ greater_equals.evaluate_zonemap_filter(mirrored, {left, right}));
+
+ // Partially overlapping ranges: every operator must keep the zone. These
are the cases that
+ // pin down which end of each range the rule reads. Taking max_a instead
of min_a for a < b, or
+ // max_b instead of min_b for a > b, still prunes here and would drop the
overlapping rows.
+ // [10, 20] vs [5, 15] overlap on [10, 15]: a = 12, b = 14 satisfies a < b
and a <= b.
+ auto left_high =
+ make_two_slot_context(make_int_zonemap(10, 20),
make_int_zonemap(5, 15), type, type);
+ EXPECT_EQ(ZoneMapFilterResult::kMayMatch,
+ less.evaluate_zonemap_filter(left_high, {left, right}));
+ EXPECT_EQ(ZoneMapFilterResult::kMayMatch,
+ less_equals.evaluate_zonemap_filter(left_high, {left, right}));
+ EXPECT_EQ(ZoneMapFilterResult::kMayMatch,
+ equals.evaluate_zonemap_filter(left_high, {left, right}));
+ // [5, 15] vs [10, 20]: a = 14, b = 12 satisfies a > b and a >= b.
+ auto right_high =
+ make_two_slot_context(make_int_zonemap(5, 15),
make_int_zonemap(10, 20), type, type);
+ EXPECT_EQ(ZoneMapFilterResult::kMayMatch,
+ greater.evaluate_zonemap_filter(right_high, {left, right}));
+ EXPECT_EQ(ZoneMapFilterResult::kMayMatch,
+ greater_equals.evaluate_zonemap_filter(right_high, {left,
right}));
+ EXPECT_EQ(ZoneMapFilterResult::kMayMatch,
+ equals.evaluate_zonemap_filter(right_high, {left, right}));
+}
+
+TEST(ExprZonemapFilterTest, SlotSlotGuardsCoverNullAndMissingStatistics) {
+ auto type = int_type();
+ auto left = make_slot(0, type);
+ auto right = make_slot(1, type);
+ FunctionComparison<LessOp, NameLess> less;
+
+ // A column with no non-null value makes the comparison NULL on every row.
Production builds
+ // such a zone map with default-constructed min/max, which carry TYPE_NULL
rather than the
+ // column type. That is why the has_not_null check has to run before the
range check: the range
+ // check asserts the bounds match the slot type and would fatal on
TYPE_NULL.
+ segment_v2::ZoneMap all_null_zonemap;
+ all_null_zonemap.has_null = true;
+ all_null_zonemap.has_not_null = false;
+ auto left_all_null =
+ make_two_slot_context(all_null_zonemap, make_int_zonemap(1, 9),
type, type);
+ EXPECT_EQ(ZoneMapFilterResult::kNoMatch,
+ less.evaluate_zonemap_filter(left_all_null, {left, right}));
+ EXPECT_EQ(0, left_all_null.stats.unusable_zonemap_eval_count);
+ // The same on the right operand: both halves of the check are
load-bearing.
+ auto right_all_null =
+ make_two_slot_context(make_int_zonemap(1, 9), all_null_zonemap,
type, type);
+ EXPECT_EQ(ZoneMapFilterResult::kNoMatch,
+ less.evaluate_zonemap_filter(right_all_null, {left, right}));
+ EXPECT_EQ(0, right_all_null.stats.unusable_zonemap_eval_count);
+
+ // The right slot is absent from the context entirely.
+ auto missing_slot = make_context(make_int_zonemap(1, 9), type);
+ EXPECT_EQ(ZoneMapFilterResult::kUnsupported,
+ less.evaluate_zonemap_filter(missing_slot, {left, right}));
+ EXPECT_EQ(1, missing_slot.stats.unusable_zonemap_eval_count);
+
+ // The right slot is present with a type but without a zone map.
+ auto missing_zonemap = make_context(make_int_zonemap(1, 9), type);
+ ZoneMapEvalContext::SlotZoneMap slot_without_zonemap;
+ slot_without_zonemap.data_type = type;
+ missing_zonemap.slots.emplace(1, std::move(slot_without_zonemap));
+ EXPECT_EQ(ZoneMapFilterResult::kUnsupported,
+ less.evaluate_zonemap_filter(missing_zonemap, {left, right}));
+ EXPECT_EQ(1, missing_zonemap.stats.unusable_zonemap_eval_count);
+
+ // pass_all marks the bounds as unusable even though has_not_null is set.
+ auto pass_all_zonemap = make_int_zonemap(1, 9);
+ pass_all_zonemap.pass_all = true;
+ auto pass_all = make_two_slot_context(make_int_zonemap(10, 20),
std::move(pass_all_zonemap),
+ type, type);
+ EXPECT_EQ(ZoneMapFilterResult::kUnsupported,
+ less.evaluate_zonemap_filter(pass_all, {left, right}));
+ EXPECT_EQ(1, pass_all.stats.unusable_zonemap_eval_count);
+}
+
+TEST(ExprZonemapFilterTest, SlotSlotHandlesTheSameSlotOnBothSides) {
+ // FE's SimplifySelfComparison does not fold NotEqualTo, so `a != a`
reaches the BE. It is never
+ // true, so returning kNoMatch is always sound; the range rule proves it
when a is a single
+ // value and stays conservative otherwise.
+ auto type = int_type();
+ auto slot = make_slot(0, type);
+ FunctionComparison<NotEqualsOp, NameNotEquals> not_equals;
+
+ auto single_value = make_context(make_int_zonemap(7, 7), type);
+ EXPECT_EQ(ZoneMapFilterResult::kNoMatch,
+ not_equals.evaluate_zonemap_filter(single_value, {slot, slot}));
+ auto range = make_context(make_int_zonemap(7, 8), type);
+ EXPECT_EQ(ZoneMapFilterResult::kMayMatch,
+ not_equals.evaluate_zonemap_filter(range, {slot, slot}));
+}
+
+TEST(ExprZonemapFilterTest,
SlotSlotWidensOnlyTheZonemapGateNotDictionaryOrBloom) {
+ // This is the regression guard for the trap in this change:
comparison_zonemap_detail's
+ // can_evaluate is shared by dictionary filtering and
can_evaluate_equality, and both dereference
+ // extract_slot_and_literal behind a DORIS_CHECK. Widening it would abort
on `a != b`, so the
+ // slot-vs-slot shape must only ever be accepted by
can_evaluate_zonemap_filter.
+ auto type = int_type();
+ auto left = make_slot(0, type);
+ auto right = make_slot(1, type);
+ FunctionComparison<NotEqualsOp, NameNotEquals> not_equals;
+
+ EXPECT_TRUE(not_equals.can_evaluate_zonemap_filter({left, right}));
+ EXPECT_FALSE(not_equals.can_evaluate_dictionary_filter({left, right}));
+
+ // The bloom gate has to be checked through EQ. can_evaluate_bloom_filter
starts with
+ // `op == Op::EQ`, so asserting it on NE would pass no matter what the
rest of the gate does.
+ FunctionComparison<EqualsOp, NameEquals> equals;
+ EXPECT_TRUE(equals.can_evaluate_zonemap_filter({left, right}));
+ EXPECT_FALSE(equals.can_evaluate_bloom_filter({left, right}));
+ EXPECT_TRUE(equals.can_evaluate_bloom_filter({left, make_int_literal(1)}));
+
+ // Slot vs literal keeps working on every gate.
+ EXPECT_TRUE(not_equals.can_evaluate_zonemap_filter({left,
make_int_literal(1)}));
+ EXPECT_TRUE(not_equals.can_evaluate_dictionary_filter({left,
make_int_literal(1)}));
+
+ // Two columns of incompatible types are rejected: the two Fields would be
compared directly and
+ // Field comparison throws on mismatched non-string types.
+ auto bigint = std::make_shared<DataTypeInt64>();
+ EXPECT_FALSE(not_equals.can_evaluate_zonemap_filter({left, make_slot(1,
bigint)}));
+}
+
+TEST(ExprZonemapFilterTest, SlotSlotBailsOutWhenAFloatingNanCountIsUnknown) {
+ // Parquet bounds omit NaN without reporting how many were skipped, so a
zone map that looks like
+ // a single point may still hide one. That would flip the NE rule from
false to true, so the
+ // slot-vs-slot path refuses to prune for every operator once either side
is marked unknown.
+ auto type = std::make_shared<DataTypeFloat64>();
+ auto left = make_slot(0, type);
+ auto right = make_slot(1, type);
+
+ auto make_double_zonemap = [](double value) {
+ segment_v2::ZoneMap zone_map;
+ zone_map.min_value = Field::create_field<TYPE_DOUBLE>(value);
+ zone_map.max_value = Field::create_field<TYPE_DOUBLE>(value);
+ zone_map.has_not_null = true;
+ return zone_map;
+ };
+
+ // Control group. Both sides are the same single value, so NE prunes
without the guard; the
+ // contrast with the loop below is what shows the guard is what changes
the outcome. The counter
+ // alone cannot show that, since every bail-out path in evaluate_slot_slot
bumps the same one.
+ FunctionComparison<NotEqualsOp, NameNotEquals> not_equals;
+ auto known =
+ make_two_slot_context(make_double_zonemap(1.0),
make_double_zonemap(1.0), type, type);
+ EXPECT_EQ(ZoneMapFilterResult::kNoMatch,
+ not_equals.evaluate_zonemap_filter(known, {left, right}));
+ EXPECT_EQ(0, known.stats.unusable_zonemap_eval_count);
+
+ auto expect_bails_out = [&](auto&& comparison, const char* predicate) {
+ for (int unknown_slot : {0, 1}) {
+ auto ctx = make_two_slot_context(make_double_zonemap(1.0),
make_double_zonemap(1.0),
+ type, type);
+ ctx.slots[unknown_slot].floating_nan_count_unknown = true;
+ EXPECT_EQ(ZoneMapFilterResult::kUnsupported,
+ comparison.evaluate_zonemap_filter(ctx, {left, right}))
+ << predicate << " must stop pruning when slot " <<
unknown_slot
+ << " has an unknown NaN count";
+ EXPECT_EQ(1, ctx.stats.unusable_zonemap_eval_count);
+ }
+ };
+
+ // All six operators bail out. On these bounds NE, LT and GT would
otherwise prune and EQ, LE
+ // and GE would otherwise keep the zone, so none of the six is vacuously
covered.
+ expect_bails_out(FunctionComparison<EqualsOp, NameEquals> {}, "a = b");
+ expect_bails_out(FunctionComparison<NotEqualsOp, NameNotEquals> {}, "a !=
b");
+ expect_bails_out(FunctionComparison<LessOp, NameLess> {}, "a < b");
+ expect_bails_out(FunctionComparison<LessOrEqualsOp, NameLessOrEquals> {},
"a <= b");
+ expect_bails_out(FunctionComparison<GreaterOp, NameGreater> {}, "a > b");
+ expect_bails_out(FunctionComparison<GreaterOrEqualsOp,
NameGreaterOrEquals> {}, "a >= b");
+}
+
+TEST(ExprZonemapFilterTest, ParquetSlotZoneMapMarksFloatingNanCountUnknown) {
Review Comment:
**Coverage: exercise both production Parquet reader paths.**
This test constructs `SlotZoneMap` directly, so it still passes if either v1
call site stops using the setter or if a reader maps one of the two slots to
the wrong Column Chunk. The new regression suite only writes native OLAP
segments, whose zone maps record `has_nan`, so it cannot reproduce the Parquet
bug this patch fixes. Please add a Parquet fixture with finite bounds that
proves two-column Row Group pruning through v1 and v2, plus a fixture such as
`a={1.0, NaN}`, `b={1.0, 1.0}` that proves both readers retain the Row Group
and return the `a != b` row when serialized bounds omit NaN. The v1 decimal
schema-evolution counterexample from the other comment also needs a
production-path regression.
--
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]
