LuciferYang commented on code in PR #67774:
URL: https://github.com/apache/doris/pull/67774#discussion_r4056483570
##########
be/src/exprs/function/functions_comparison.h:
##########
@@ -314,42 +342,94 @@ inline ZoneMapFilterResult evaluate(const
ZoneMapEvalContext& ctx, const VExprSP
return unsupported_zonemap_filter(ctx);
}
- const auto effective_op = slot_literal->literal_on_left ? symmetric_op(op)
: op;
- const auto& literal = slot_literal->literal;
+ const auto effective_op = slot_literal.literal_on_left ? symmetric_op(op)
: op;
+ const auto& literal = slot_literal.literal;
const bool literal_is_nan = literal.is_nan();
const bool hidden_nan_can_match = (effective_op == Op::EQ &&
literal_is_nan) ||
(effective_op == Op::NE &&
!literal_is_nan) ||
(effective_op == Op::GT &&
!literal_is_nan) ||
effective_op == Op::GE;
- if (ctx.floating_nan_count_unknown(slot_literal->slot_index) &&
hidden_nan_can_match) {
+ if (ctx.floating_nan_count_unknown(slot_literal.slot_index) &&
hidden_nan_can_match) {
// Parquet bounds omit NaNs, so only operators that cannot match a
hidden NaN may prune.
return unsupported_zonemap_filter(ctx);
}
- switch (effective_op) {
- case Op::EQ:
- return literal < zone_map.min_value || zone_map.max_value < literal
- ? ZoneMapFilterResult::kNoMatch
- : ZoneMapFilterResult::kMayMatch;
- case Op::NE:
- return zone_map.min_value == literal && zone_map.max_value == literal
- ? ZoneMapFilterResult::kNoMatch
- : ZoneMapFilterResult::kMayMatch;
- case Op::LT:
- return zone_map.min_value >= literal ? ZoneMapFilterResult::kNoMatch
- : ZoneMapFilterResult::kMayMatch;
- case Op::LE:
- return zone_map.min_value > literal ? ZoneMapFilterResult::kNoMatch
- : ZoneMapFilterResult::kMayMatch;
- case Op::GT:
- return zone_map.max_value <= literal ? ZoneMapFilterResult::kNoMatch
- : ZoneMapFilterResult::kMayMatch;
- case Op::GE:
- return zone_map.max_value < literal ? ZoneMapFilterResult::kNoMatch
- : ZoneMapFilterResult::kMayMatch;
+ return range_vs_range_no_match(zone_map.min_value, zone_map.max_value,
literal, literal,
+ effective_op)
+ ? ZoneMapFilterResult::kNoMatch
+ : ZoneMapFilterResult::kMayMatch;
+}
+
+inline ZoneMapFilterResult evaluate_slot_slot(const ZoneMapEvalContext& ctx,
+ const expr_zonemap::SlotSlot&
slot_slot, Op op) {
+ const auto left_type = expr_zonemap::fetch_compatible_slot_type(ctx,
slot_slot.left_slot_index,
+
slot_slot.left_type);
+ const auto right_type = expr_zonemap::fetch_compatible_slot_type(
+ ctx, slot_slot.right_slot_index, slot_slot.right_type);
+ if (left_type == nullptr || right_type == nullptr) {
+ // The context skips a slot entirely when the segment cannot apply
predicates on it.
+ return unsupported_zonemap_filter(ctx);
}
+ const auto left_zone_map = ctx.zone_map(slot_slot.left_slot_index);
+ const auto right_zone_map = ctx.zone_map(slot_slot.right_slot_index);
+ if (left_zone_map == nullptr || right_zone_map == nullptr) {
+ // A slot can be present with a data type but no zone map, so this is
a separate check.
+ return unsupported_zonemap_filter(ctx);
+ }
+ // A column holding no non-null value makes the comparison NULL on every
row, which never
+ // satisfies a WHERE conjunct. This must run before the range checks
below: an all-null zone map
+ // leaves min/max default-constructed as TYPE_NULL, which the range checks
would fatal on.
+ if (!left_zone_map->has_not_null || !right_zone_map->has_not_null) {
+ return ZoneMapFilterResult::kNoMatch;
+ }
+ if (!expr_zonemap::range_stats_usable_for_zonemap(*left_zone_map,
left_type) ||
+ !expr_zonemap::range_stats_usable_for_zonemap(*right_zone_map,
right_type)) {
+ return unsupported_zonemap_filter(ctx);
+ }
+ // A native string zone-map max is truncated to MAX_ZONE_MAP_INDEX_SIZE,
then its last byte is
+ // incremented (which wraps on 0xff), so a bound of exactly that length is
not a reliable fence
+ // for a two-sided proof. A shorter bound is the untruncated exact value;
the increment never
+ // changes the length, so this also rejects an already-wrapped bound
without a truncation flag.
+ // The test only decides whether to trust the bound as a fence. A
Parquet-sourced string bound
+ // shorter than the cap is not necessarily exact, but it is a
spec-conservative min/max, which
+ // is all a range proof needs; do not tighten this to assume exactness
below the cap.
+ // data_types_compatible pairs strings only with strings, so checking one
side's type is enough
+ // to know all four bounds are strings.
+ if (is_string_type(remove_nullable(left_type)->get_primitive_type())) {
+ auto is_untruncated = [](const Field& f) {
+ return f.get<TYPE_STRING>().size() < MAX_ZONE_MAP_INDEX_SIZE;
+ };
+ if (!is_untruncated(left_zone_map->min_value) ||
+ !is_untruncated(left_zone_map->max_value) ||
+ !is_untruncated(right_zone_map->min_value) ||
+ !is_untruncated(right_zone_map->max_value)) {
+ return unsupported_zonemap_filter(ctx);
+ }
+ }
+ // Parquet bounds omit NaN without recording how many were skipped, so a
zone map that looks
+ // like a single point may still hide one, which would flip the NE rule
from false to true. The
+ // slot-vs-literal path can reason per operator because it knows whether
the literal is NaN;
+ // with two slots there is no literal, so bail out for every operator
instead.
+ if (ctx.floating_nan_count_unknown(slot_slot.left_slot_index) ||
+ ctx.floating_nan_count_unknown(slot_slot.right_slot_index)) {
+ return unsupported_zonemap_filter(ctx);
+ }
+ return range_vs_range_no_match(left_zone_map->min_value,
left_zone_map->max_value,
Review Comment:
Makes sense, dropping it. I removed the prepared BE special case, so this PR
keeps only the existing conservative same-slot behavior.
You are right about reachability and I had it wrong: `a != a` is
`Not(EqualTo(a, a))`, SimplifySelfComparison folds the inner equality, so it
does not arrive at the BE as a slot-vs-slot NotEqualTo. I corrected the stale
note in `SlotSlotHandlesTheSameSlotOnBothSides` (f5040176207) so it no longer
claims the predicate reaches the BE; the test now just pins the conservative
fallback (single value proves kNoMatch via the point-range rule, a range stays
kMayMatch).
Agreed the `kMayMatch` fallback only costs a scan and never wrong rows, and
that self-comparison belongs in FE with NULL semantics preserved. If an EXPLAIN
ever shows a real gap I will open a separate FE issue for it. #67995 and #68118
stand as the correctness dependencies.
--
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]
