LuciferYang commented on code in PR #67774:
URL: https://github.com/apache/doris/pull/67774#discussion_r3981190422
##########
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:
Agreed, and this is the comment that paid for itself: writing the test you
asked for is what showed that my first fix for the other comment would have
aborted the BE rather than degraded. So I am not going to argue the gap away.
What I added is honest about its level rather than pretending to cover the
reader. `SlotSlotComparesDecimalBoundsInOneDomain` exercises the decimal path
for real, both sides `DECIMAL64(18,2)` so `Field` comparison runs on
`Decimal64` rather than a widened int, with a separated pair, a pair touching
at the boundary that separates `lmin >= rmax` from `lmin > rmax`, and an
overlapping pair. That pins the interval rule for decimals, which had no
coverage before.
What it does not do is prove the production binding, and you are right about
why: `make_two_slot_context` assigns `data_type` directly instead of going
through `set_data_type_from_parquet`, so it cannot express what either reader
records. A v1 or v2 call site that stopped using the setter, or mapped a slot
to the wrong Column Chunk, would still pass.
Closing that needs Parquet input this PR cannot synthesize: a file whose
DECIMAL column carries a scale the table does not declare, and a file whose
DOUBLE column holds `{1.0, NaN}` with bounds that omit the NaN.
`be/test/format/parquet/parquet_expr_test.cpp` is the right home, it already
drives the v1 expression path against a real `ParquetReader`, but its fixture
opens a checked-in file and hardcodes the table side at `DECIMAL64(10,2)` /
`DECIMAL128I(18,6)`, so the fixtures have to come with new test data files
rather than being constructed in the test.
Two things worth knowing while that is outstanding. The rule the v1
hidden-NaN fix restores is pinned directly by
`ParquetSlotZoneMapMarksFloatingNanCountUnknown`, so a call site that drops the
setter fails there rather than going unnoticed; that is weaker than a reader
test but it is not nothing. And the same file already contains two tests named
for v1 float predicate push-down, `test_expr_push_down_gt_float` and
`test_expr_push_down_ge_double`, which build a statistics callback and an
expression context and then assert only that the predicate is not constant, so
the reader is never invoked. They are the natural place to start, and reviving
them is the same piece of work as your fixtures. I would rather file that as
its own issue than bolt a half version onto this PR.
--
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]
