github-actions[bot] commented on code in PR #66422:
URL: https://github.com/apache/doris/pull/66422#discussion_r3712393441
##########
be/src/storage/predicate/in_list_predicate.h:
##########
@@ -623,6 +629,12 @@ class InListPredicateBase final : public ColumnPredicate {
}
void _update_min_max(const T& value) {
+ if constexpr (Type == TYPE_FLOAT || Type == TYPE_DOUBLE) {
+ if (std::isnan(value)) {
+ _contains_nan = true;
+ return;
Review Comment:
[P1] Preserve NaN in the generic IN range
Returning here leaves the predicate min/max at finite sentinels for `IN
(NaN)` (and omits NaN from a mixed set's maximum), but those fields are also
used by Doris's own segment/page ZoneMap and delete-pruning paths. Native
ZoneMaps restore NaN as the maximum, so the truncated predicate range can skip
a page whose NaN matches the IN set; `evaluate_del` can likewise mark `{100,
NaN}` fully deleted for `NOT IN (0, NaN)` even though its NaN row must remain.
Please set `_contains_nan` without losing the existing NaN maximum, or make
both generic consumers honor the flag, and add FLOAT/DOUBLE native-storage
coverage.
##########
be/src/exprs/function/functions_comparison.h:
##########
@@ -314,6 +314,11 @@ inline ZoneMapFilterResult evaluate(const
ZoneMapEvalContext& ctx, const VExprSP
const auto effective_op = slot_literal->literal_on_left ? symmetric_op(op)
: op;
const auto& literal = slot_literal->literal;
+ if (effective_op == Op::EQ &&
ctx.floating_nan_count_unknown(slot_literal->slot_index) &&
Review Comment:
[P1] Fence the other NaN-sensitive comparisons
Limiting the new unknown-NaN check to `EQ` still lets valid finite-only
Parquet bounds drop matches. For physical `{0, NaN}` with bounds `[0,0]`, this
evaluator returns no-match for `c != 0`, `c > 1`, and `c >= 1`, while Doris
residual comparison accepts the NaN row; reversed operands reach the same
`GT`/`GE` cases. The legacy `ComparisonPredicateBase::camp_field` path has the
same gap and can prune first. Please apply the operator-aware NaN fence to both
engines and cover FLOAT/DOUBLE footer and page pruning, including
literal-on-left forms.
##########
be/src/exprs/expr_zonemap_filter.cpp:
##########
@@ -261,6 +279,15 @@ ZoneMapFilterResult eval_in_zonemap(const
ZoneMapEvalContext& ctx, const VExprSP
DORIS_CHECK(field_types_compatible(min_value.get_type(),
data_type->get_primitive_type()));
DORIS_CHECK(field_types_compatible(max_value.get_type(),
data_type->get_primitive_type()));
+ if (!is_not_in && ctx.floating_nan_count_unknown(slot->column_id()) &&
Review Comment:
[P1] Keep hidden NaNs for NOT IN too
The `!is_not_in` restriction leaves a false-negative case in the shared
V1/V2 footer and page evaluator. For physical `{0, NaN}` with Parquet bounds
`[0,0]`, `c NOT IN (0)` must return the NaN row, but the later singleton-range
branch sees `0` covered by the set and returns no-match for the whole unit.
Please fall back when NaN presence is unknown and the NOT-IN set does not
contain NaN, while preserving the existing NULL-set behavior, and add
FLOAT/DOUBLE V1/V2 coverage.
##########
be/src/exprs/expr_zonemap_filter.cpp:
##########
@@ -261,6 +279,15 @@ ZoneMapFilterResult eval_in_zonemap(const
ZoneMapEvalContext& ctx, const VExprSP
DORIS_CHECK(field_types_compatible(min_value.get_type(),
data_type->get_primitive_type()));
DORIS_CHECK(field_types_compatible(max_value.get_type(),
data_type->get_primitive_type()));
+ if (!is_not_in && ctx.floating_nan_count_unknown(slot->column_id()) &&
+ std::ranges::any_of(values, [&](const Field& value) {
Review Comment:
[P2] Precompute NaN membership before page pruning
This scan runs before the existing `> 64` range-only cutoff, and
`eval_in_zonemap` is invoked once per surviving row group and once per Parquet
page in both V1 and V2. A large floating IN/runtime-filter set without NaN
therefore performs O(set size) Field checks per page, turning metadata pruning
into O(pages * set size) work; when NaN is present, the early return also
bypasses the later all-null rejection. Please materialize a `contains_nan` bit
once (or use an explicit precomputed invariant), make this check O(1), and
apply the fallback only after trustworthy null-only pruning.
--
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]
