LuciferYang commented on code in PR #67774:
URL: https://github.com/apache/doris/pull/67774#discussion_r3981184916
##########
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
Review Comment:
Confirmed, and thanks. I traced each step: `_type_matches` compares only
`get_primitive_type()`, `parse_min_max_value` builds its converter with
`col_schema->data_type` as both source and destination so the bounds land in
the file domain, and `Field::operator<=>` for decimals compares unscaled
payloads without aligning scale. Your example prunes a matching row group.
One note on scope: this is not new in this PR. `WHERE a < 2.00` on a
scale-mismatched v1 file already compares a literal in the expression domain
against a bound in the file domain, so the single-column path has the same hole
today. This PR extends the same unsound comparison to two-slot shapes, so it
belongs here.
Fixed in ece564a24ef, and not the way I first tried. My first attempt
recorded `col_schema->data_type` in the context so that the existing
`fetch_compatible_slot_type` check would reject the mismatch. That is worse
than the bug: `fetch_compatible_slot_type` does
`DORIS_CHECK(data_types_compatible(...))` rather than returning nullptr, so a
real scale mismatch would have aborted the BE instead of degrading. I only
found that because I wrote a test for it, which is your second comment's point
exactly.
What landed instead is your first suggestion: gate at the two v1
context-construction sites and leave the zone map out unless
`data_types_compatible(col_schema->data_type, slot->type())` holds. That keeps
`ctx.data_type(cid) == slot->type()`, so the `DORIS_CHECK` invariant still
cannot fire, and it is the same predicate `format_v2` already uses at
`parquet_statistics.cpp:1136`, so v1 becomes consistent rather than getting a
new rule.
The gate costs pruning in three places, all now stated in the PR
description: a DATETIMEV2 or TIMESTAMPTZ column whose declared scale differs
from the Parquet unit loses expression zone map pruning entirely, which is the
widest practical case; a decimal pair differing only in precision is
domain-identical yet also skipped; and IS NULL / IS NOT NULL pruning goes with
it, since a zone map carrying only the null flags leaves min/max at `TYPE_NULL`
and any range predicate would then fail `range_stats_usable_for_zonemap`'s type
check.
One related hole I did not touch, because it predates this change and needs
its own investigation: unsigned INTEGER logical types are marked
`is_type_compatibility` in `schema_desc.cpp`, so the file type equals the
widened table type and passes this gate, while `parse_min_max_value`
reinterprets the statistic bytes as signed before widening.
--
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]
