github-actions[bot] commented on code in PR #67774:
URL: https://github.com/apache/doris/pull/67774#discussion_r4036614737
##########
be/src/exprs/function/functions_comparison.h:
##########
@@ -377,6 +436,35 @@ 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);
+ // A string/char/varchar zone-map max is truncated to
MAX_ZONE_MAP_INDEX_SIZE and then bumped by
+ // one on its last byte (modify_index_before_flush), which wraps when that
byte is 0xff; a STRING
+ // can hold 0xff via unhex, and truncation leaves no provenance to detect.
The truncated max is
+ // then no longer a reliable upper bound, which a two-sided slot-vs-slot
proof relies on. Reject
+ // string pairs here; column-vs-column string comparison is narrow enough
to leave unpruned. The
+ // slot-vs-literal path is unaffected: the literal is compared against a
single bound, not paired
+ // with another truncated bound.
+ if
(is_string_type(remove_nullable(slot_slot->left_type)->get_primitive_type()) ||
Review Comment:
[P1] Exclude VARBINARY from slot-slot zone-map evaluation
With `enable_mapping_varbinary`, an unannotated Parquet BYTE_ARRAY is
exposed as VARBINARY, but the v1 `parse_min_max_value()` path keeps BYTE_ARRAY
statistics in `ColumnString`; the consistent converter therefore produces
TYPE_STRING Fields. This gate accepts two VARBINARY slots, and row-group
evaluation then calls `range_stats_usable_for_zonemap()` with a VARBINARY
context, whose STRING-vs-VARBINARY `DORIS_CHECK` aborts the query instead of
falling back to a scan. Please reject VARBINARY here until v1 produces
correctly typed bounds, and add a raw BYTE_ARRAY/VARBINARY fixture.
##########
regression-test/suites/query_p0/expr_zonemap/test_expr_zonemap_pruning.groovy:
##########
@@ -329,4 +329,333 @@ suite("test_expr_zonemap_pruning") {
"""
assertEquals(0L, isNotNullPrunedRows[0][1] as long)
assertExprZonemapPruned(isNotNullToken)
+
+ // Column-vs-column comparisons. A predicate over two columns of the same
table never became a
+ // ColumnPredicate, so it reaches the scanner as a common expression and
is evaluated against the
+ // segment zone map of both slots at once.
+ sql """ DROP TABLE IF EXISTS test_expr_zonemap_pruning_two_columns """
+ sql """
+ CREATE TABLE test_expr_zonemap_pruning_two_columns (
+ id INT,
+ lo INT,
+ hi INT,
+ alt INT,
+ expected INT,
+ actual INT
+ ) ENGINE=OLAP
+ DUPLICATE KEY(id)
+ DISTRIBUTED BY HASH(id) BUCKETS 1
+ PROPERTIES (
+ "replication_allocation" = "tag.location.default: 1",
+ "disable_auto_compaction" = "true"
+ )
+ """
+ // lo lands in [0, 4095] and hi in [10000, 14095], so the two ranges are
fully separated. alt
+ // lands in [0, 7095] and equals lo on even rows and lo + 3000 on odd
ones, so lo vs alt cannot
+ // be decided from the bounds and every row has to be evaluated. expected
and actual are both
+ // the constant 7.
+ sql """
+ INSERT INTO test_expr_zonemap_pruning_two_columns
+ SELECT CAST(number AS INT),
+ CAST(number AS INT),
+ CAST(number + 10000 AS INT),
+ IF(number % 2 = 0, CAST(number AS INT), CAST(number + 3000 AS
INT)),
+ 7,
+ 7
+ FROM numbers("number" = "4096")
+ """
+ sql """ sync """
+
+ // Runs the same query with expr zonemap pruning on and off and asserts
the two agree. The
+ // counter only shows that pruning fired; this is what shows it fired
correctly.
+ def assertSameWithAndWithoutPruning = { String predicate ->
+ sql """ set enable_expr_zonemap_filter = false """
+ def withoutPruning = sql """
+ SELECT COUNT(*) FROM test_expr_zonemap_pruning_two_columns WHERE
${predicate}
+ """
+ sql """ set enable_expr_zonemap_filter = true """
+ def withPruning = sql """
+ SELECT COUNT(*) FROM test_expr_zonemap_pruning_two_columns WHERE
${predicate}
+ """
+ assertEquals(withoutPruning[0][0] as long, withPruning[0][0] as long)
+ return withPruning[0][0] as long
+ }
+
+ def assertTwoColumnPruned = { String predicate, String label ->
+ def token = "expr_zonemap_pruning_two_columns_" + label + "_" +
UUID.randomUUID().toString()
+ def rows = sql """
+ SELECT '${token}', COUNT(*) FROM
test_expr_zonemap_pruning_two_columns
+ WHERE ${predicate}
+ """
+ assertEquals(0L, rows[0][1] as long)
+ assertExprZonemapPruned(token)
+ assertEquals(0L, assertSameWithAndWithoutPruning(predicate))
+ }
+
+ // lo > hi and lo >= hi: rejected because min(hi) is already above max(lo).
+ assertTwoColumnPruned("lo > hi", "gt")
+ assertTwoColumnPruned("lo >= hi", "ge")
+ // hi < lo and hi <= lo: the mirrored rules.
+ assertTwoColumnPruned("hi < lo", "lt")
+ assertTwoColumnPruned("hi <= lo", "le")
+ // lo = hi: the ranges are disjoint, so no row can be equal.
+ assertTwoColumnPruned("lo = hi", "eq")
+ // expected != actual: both columns collapse to the single value 7, which
is the only shape that
+ // lets != prune.
+ assertTwoColumnPruned("expected != actual", "ne")
+
+ // Overlapping ranges must survive, and the row counts must be exact.
These are the cases that
+ // catch a rule reading the wrong end of a range: lo in [0, 4095] against
alt in [0, 7095] cannot
+ // be separated by the bounds, so all of these have to fall through to
per-row evaluation.
+ assertEquals(2048L, assertSameWithAndWithoutPruning("lo < alt"))
+ assertEquals(2048L, assertSameWithAndWithoutPruning("lo != alt"))
+ assertEquals(2048L, assertSameWithAndWithoutPruning("lo = alt"))
+ assertEquals(4096L, assertSameWithAndWithoutPruning("lo <= alt"))
+ assertEquals(0L, assertSameWithAndWithoutPruning("lo > alt"))
+ assertEquals(4096L, assertSameWithAndWithoutPruning("lo >= alt - 3000"))
+ // A cast on either side is rejected by the capability gate, so this must
still return the right
+ // answer rather than being pruned on raw bounds.
+ assertEquals(4096L, assertSameWithAndWithoutPruning("lo < CAST(hi AS
BIGINT)"))
+
+ // One side partially NULL. min/max summarize the non-null values only,
and a NULL row makes the
+ // comparison NULL, which never satisfies the conjunct, so the separated
ranges still prune.
+ sql """ DROP TABLE IF EXISTS test_expr_zonemap_pruning_two_columns_nulls
"""
+ sql """
+ CREATE TABLE test_expr_zonemap_pruning_two_columns_nulls (
+ id INT,
+ lo INT,
+ hi INT
+ ) ENGINE=OLAP
+ DUPLICATE KEY(id)
+ DISTRIBUTED BY HASH(id) BUCKETS 1
+ PROPERTIES (
+ "replication_allocation" = "tag.location.default: 1",
+ "disable_auto_compaction" = "true"
+ )
+ """
+ sql """
+ INSERT INTO test_expr_zonemap_pruning_two_columns_nulls
+ SELECT CAST(number AS INT),
+ IF(number % 8 = 0, NULL, CAST(number AS INT)),
+ CAST(number + 10000 AS INT)
+ FROM numbers("number" = "4096")
+ """
+ sql """ sync """
+
+ def twoColumnNullToken =
+ "expr_zonemap_pruning_two_columns_null_" +
UUID.randomUUID().toString()
+ def twoColumnNullRows = sql """
+ SELECT '${twoColumnNullToken}', COUNT(*) FROM
test_expr_zonemap_pruning_two_columns_nulls
+ WHERE lo > hi
+ """
+ assertEquals(0L, twoColumnNullRows[0][1] as long)
+ assertExprZonemapPruned(twoColumnNullToken)
+
+ // A column with no non-null value at all makes the comparison NULL on
every row.
+ sql """ DROP TABLE IF EXISTS
test_expr_zonemap_pruning_two_columns_all_null """
+ sql """
+ CREATE TABLE test_expr_zonemap_pruning_two_columns_all_null (
+ id INT,
+ lo INT,
+ hi INT
+ ) ENGINE=OLAP
+ DUPLICATE KEY(id)
+ DISTRIBUTED BY HASH(id) BUCKETS 1
+ PROPERTIES (
+ "replication_allocation" = "tag.location.default: 1",
+ "disable_auto_compaction" = "true"
+ )
+ """
+ sql """
+ INSERT INTO test_expr_zonemap_pruning_two_columns_all_null
+ SELECT CAST(number AS INT), NULL, CAST(number AS INT)
+ FROM numbers("number" = "4096")
+ """
+ sql """ sync """
+
+ def allNullToken =
+ "expr_zonemap_pruning_two_columns_all_null_" +
UUID.randomUUID().toString()
+ def allNullRows = sql """
+ SELECT '${allNullToken}', COUNT(*) FROM
test_expr_zonemap_pruning_two_columns_all_null
+ WHERE lo < hi
+ """
+ assertEquals(0L, allNullRows[0][1] as long)
+ assertExprZonemapPruned(allNullToken)
+
+ // Same as assertExprZonemapPruned but returns the count, so a case can
pin the exact number of
+ // segments that had to be dropped instead of only that pruning happened
at all.
+ def filteredSegmentsOf = { String token ->
+ long filteredSegments = 0
+ for (int retry = 0; retry < 20; ++retry) {
+ String profile = getProfileByToken(token).toString()
+ filteredSegments = counterSum(profile,
"ExprZoneMapFilteredSegments")
+ if (filteredSegments > 0) {
+ return filteredSegments
+ }
+ Thread.sleep(500)
+ }
+ return filteredSegments
+ }
+
+ // Three loads, one segment each, each segment holding a constant in both
columns. This is the
+ // shape where a cross-column rule has to decide per segment rather than
per table, so the exact
+ // number of dropped segments is the assertion that matters.
+ //
+ // batch | a | b | a != b | a = b
+ // 1 | a | a | both collapse | overlap, keep
+ // 2 | a | b | disjoint,keep | disjoint, drop
+ // 3 | b | b | both collapse | overlap, keep
+ sql """ DROP TABLE IF EXISTS test_expr_zonemap_pruning_per_segment """
+ sql """
+ CREATE TABLE test_expr_zonemap_pruning_per_segment (
+ id INT,
+ a VARCHAR(32),
+ b VARCHAR(32)
+ ) ENGINE=OLAP
+ DUPLICATE KEY(id)
+ DISTRIBUTED BY HASH(id) BUCKETS 1
+ PROPERTIES (
+ "replication_allocation" = "tag.location.default: 1",
+ "disable_auto_compaction" = "true"
+ )
+ """
+ sql """
+ INSERT INTO test_expr_zonemap_pruning_per_segment
+ SELECT CAST(number AS INT), 'a', 'a' FROM numbers("number" = "1024")
+ """
+ sql """ sync """
+ sql """
+ INSERT INTO test_expr_zonemap_pruning_per_segment
+ SELECT CAST(number AS INT), 'a', 'b' FROM numbers("number" = "1024")
+ """
+ sql """ sync """
+ sql """
+ INSERT INTO test_expr_zonemap_pruning_per_segment
+ SELECT CAST(number AS INT), 'b', 'b' FROM numbers("number" = "1024")
+ """
+ sql """ sync """
+
+ def perSegmentNeToken = "expr_zonemap_pruning_per_segment_ne_" +
UUID.randomUUID().toString()
+ def perSegmentNeRows = sql """
+ SELECT '${perSegmentNeToken}', COUNT(*) FROM
test_expr_zonemap_pruning_per_segment
+ WHERE a != b
+ """
+ assertEquals(1024L, perSegmentNeRows[0][1] as long)
+ assertEquals(2L, filteredSegmentsOf(perSegmentNeToken))
Review Comment:
[P1] Use a supported type for the segment-counter fixture
Both columns in this new table are VARCHAR, but `can_evaluate_slot_slot()`
now deliberately returns false whenever either operand is CHAR/VARCHAR/STRING.
The segment context therefore never evaluates these predicates and
`ExprZoneMapFilteredSegments` remains zero, so this `2L` assertion and the `1L`
assertion below are guaranteed to fail even though residual row counts are
correct. Keep the string safety gate and use a supported non-string pair (for
example INT values) for this pruning-counter 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]
