LuciferYang commented on code in PR #67774:
URL: https://github.com/apache/doris/pull/67774#discussion_r4033692719
##########
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:
Fixed by #67779, merged to master as 1a141d30e91, and this branch is rebased
on it. `ZoneMap::from_proto` now marks a legacy float/double map that has
`has_not_null` but no `has_nan` field as `pass_all`, and
`range_stats_usable_for_zonemap` rejects `pass_all` before any bound is read,
so the slot-vs-slot proof inherits the conservative behavior with no change
here.
##########
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:
Tracked as #67995. The native segment and page builders record the
placeholder summary for the commit-TSO, version, and binlog-TSO ordinals; the
fix is a schema-derived effective-read-time summary in the builders, which this
proof then inherits. Kept out of this PR because it also affects single-slot
expression pruning and the ColumnPredicate path, and the correct signal is the
column ordinal, not the reader type. Merge-ordered after it.
##########
be/src/exprs/function/functions_comparison.h:
##########
@@ -377,6 +436,24 @@ inline bool can_evaluate(const VExprSPtrs& arguments) {
return true;
}
+// Accept a comparison whose both operands are slot references. Kept separate
from can_evaluate on
+// purpose: can_evaluate also gates dictionary filtering and
can_evaluate_equality, and both of
+// those dereference extract_slot_and_literal behind a DORIS_CHECK, so
widening it would abort on a
+// slot-vs-slot expression. Only can_evaluate_zonemap_filter ORs this in.
+inline bool can_evaluate_slot_slot(const VExprSPtrs& arguments) {
+ auto slot_slot = expr_zonemap::extract_slot_and_slot(arguments);
+ if (!slot_slot.has_value()) {
+ return false;
+ }
+ DORIS_CHECK(slot_slot->left_type != nullptr);
+ DORIS_CHECK(slot_slot->right_type != nullptr);
+ // The two zone maps' Fields are compared directly and Field comparison
throws on mismatched
+ // non-string types, so reject incompatible column pairs here. A pair
differing only by width or
+ // decimal scale never reaches this point anyway, because the optimizer
inserts a cast and a
+ // cast is not a VSlotRef.
+ return expr_zonemap::data_types_compatible(slot_slot->left_type,
slot_slot->right_type);
Review Comment:
Will exclude string-family pairs from `can_evaluate_slot_slot`.
`modify_index_before_flush` bumps a 512-byte truncated max by one on the last
byte and wraps when that byte is 0xff, which `unhex` can produce in a
STRING/VARCHAR value, and truncation leaves no provenance in the ZoneMap to
detect, so a string maximum can't be trusted for a two-sided proof.
Column-vs-column string comparison is narrow enough to drop rather than carry
that risk; adding a unit test that the gate rejects string pairs. The general
single-slot truncated-max wrap is a separate, pre-existing concern.
##########
be/src/format/parquet/vparquet_reader.cpp:
##########
@@ -1665,14 +1670,22 @@ Status
ParquetReader::_process_expr_zonemap_filter(const tparquet::RowGroup& row
}
auto* slot = _tuple_descriptor->slots()[cid];
ZoneMapEvalContext::SlotZoneMap slot_zone_map;
- slot_zone_map.data_type = slot->type();
+ slot_zone_map.set_data_type_from_parquet(slot->type());
if (!_exists_in_file(slot->col_name()) || !_type_matches(cid)) {
ctx.slots.emplace(cid, std::move(slot_zone_map));
continue;
}
const auto& file_col_name =
_table_info_node_ptr->children_file_column_name(slot->col_name());
const FieldSchema* col_schema =
_file_metadata->schema().get_column(file_col_name);
+ // parse_min_max_value decodes the bounds in the file's own logical
type, while _type_matches
+ // only compares primitive types. A DECIMAL bound decoded at the
file's scale would then be
+ // compared against the table's scale as if the payloads shared a
domain, so leave the zone
+ // map out unless the two types agree exactly.
Review Comment:
Filed as #68118. Two parts there: v1 lacks the DST-rollback monotonic fence
v2 applies via `timestamp_min_max_is_safe` /
`utc_timestamp_range_is_monotonic`, and the INT96 `min == max` validation at
`parquet_predicate.h:344-345` is dead because both operands read `min_field`.
The fix leaves the timestamp zone map unset when the converted range is
unusable, so this proof inherits safety. Merge-ordered after #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]
