HappenLee commented on code in PR #67774:
URL: https://github.com/apache/doris/pull/67774#discussion_r4056449059
##########
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:
@LuciferYang Let's skip this BE-specific optimization in this PR and treat
this finding as non-blocking. No need to fold the prepared BE special case into
this change.
The reachability argument needs correction: at the reviewed head,
[LogicalPlanBuilder](https://github.com/apache/doris/blob/6de0272c64e35f9f357c7727af5f762f53ca51b1/fe/fe-core/src/main/java/org/apache/doris/nereids/parser/LogicalPlanBuilder.java#L3117)
parses `a != a` as `Not(EqualTo(a, a))`, and
[SimplifySelfComparison](https://github.com/apache/doris/blob/6de0272c64e35f9f357c7727af5f762f53ca51b1/fe/fe-core/src/main/java/org/apache/doris/nereids/rules/expression/rules/SimplifySelfComparison.java#L55)
already rewrites the inner equality using `trueOrNull`. The absence of a
standalone `NotEqualTo` branch does not establish that this predicate survives
FE optimization and reaches BE. An actual SQL/EXPLAIN case is needed to
demonstrate any remaining gap.
Self-comparison simplification is better handled in FE, with NULL semantics
preserved: `a != a` is FALSE for non-NULL values and NULL for NULL values;
neither passes a WHERE filter, while a projected expression must retain the
NULL result. If a real optimization gap remains, let's track it separately in
FE.
For this particular finding, the BE `kMayMatch` fallback is conservative: it
can miss pruning and perform extra scanning, but it does not produce incorrect
query results. This decision does not waive the separately tracked correctness
dependencies #67995 and #68118.
--
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]
