github-actions[bot] commented on code in PR #67774:
URL: https://github.com/apache/doris/pull/67774#discussion_r4060255032
##########
be/src/exprs/function/functions_comparison.h:
##########
@@ -734,8 +844,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] Validate v1 fixed-width statistics before enabling this shape. This
newly makes a bare `a < b` zone-map evaluable, and the v1 row-group path then
parses both footer summaries. `read_column_stats` accepts their serialized
strings without width checks; `parse_min_max_value` reads
INT32/INT64/FLOAT/DOUBLE through fixed-width dereferences, while
FIXED_LEN_BYTE_ARRAY/INT96 guard lengths only with release-disabled `DCHECK`s
before copying into schema-sized buffers. A malformed one-byte INT32 statistic
can therefore be read out of bounds, and an oversized fixed-length statistic
can write past its allocation for a query that did not enter this path before
this PR. Please validate both encoded widths and fall back without a zone map,
or keep v1 slot-slot pruning disabled until the shared parser is safe.
##########
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) {
Review Comment:
[P2] Wait for a complete profile before returning an exact total.
`getProfileByToken` fetches the first matching profile without checking
`Profile Completion State`, and this branch returns on the first positive
counter. The new callers then require final totals of 2, 1, and 3, so a
partially reported profile can expose 1 first and fail an otherwise-correct
test. Please use the completion-aware `ProfileAction.getProfileBySql(...)` path
(or explicitly wait for COMPLETE) before summing and returning the counter.
--
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]
