github-actions[bot] commented on code in PR #67774:
URL: https://github.com/apache/doris/pull/67774#discussion_r4011865361
##########
be/src/exprs/function/functions_comparison.h:
##########
@@ -734,8 +811,11 @@ class FunctionComparison : public IFunction {
}
bool can_evaluate_zonemap_filter(const VExprSPtrs& arguments) const
override {
- return comparison_zonemap_detail::op_from_name(name).has_value() &&
- comparison_zonemap_detail::can_evaluate(arguments);
+ if (!comparison_zonemap_detail::op_from_name(name).has_value()) {
+ return false;
+ }
+ return comparison_zonemap_detail::can_evaluate(arguments) ||
+ comparison_zonemap_detail::can_evaluate_slot_slot(arguments);
Review Comment:
[P1] Keep synthesized slots out of raw native zone-map pruning
This gate also accepts bare comparisons involving hidden columns whose
on-disk values are only placeholders. Stream-sequence predicates are rewritten
to commit/binlog TSO (stored as `[0,0]` or all-NULL), and
`__DORIS_VERSION_COL__` is stored as 0, but row evaluation later substitutes
the rowset TSO/version. Both segment and page expression pruning can trust the
placeholders: alias pushdown can collapse two sequence aliases to one
binlog-TSO ordinal, so the page path treats its all-NULL map as a single-slot
proof, while the segment path can also cache a raw commit-TSO reader before the
const-aware read. These paths can drop matching rows. Please gate or synthesize
all three ordinals centrally for both contexts, with stream-history,
alias-generated stream-incremental, and hidden-version regression coverage.
##########
be/src/exprs/function/functions_comparison.h:
##########
@@ -314,42 +341,74 @@ 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);
+ }
+ // 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) ||
Review Comment:
[P1] Preserve unknown NaN provenance for legacy native zone maps
This guard is populated by Parquet callers, but native segment contexts
leave it false and trust `ZoneMapPB::has_nan`. That optional field was added in
2025 while older writers already emitted FLOAT/DOUBLE zone maps, so an old
finite-looking `[1,1]` summary can still hide NaN. The new `a != b` point-range
proof then returns `kNoMatch` although a stored NaN differs from 1. Please
retain protobuf presence/version when decoding native floating summaries and
fall back when the field is absent, with a legacy/mixed-version compatibility
test.
--
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]
