seawinde commented on code in PR #61345:
URL: https://github.com/apache/doris/pull/61345#discussion_r3202387860
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/expression/rules/AddMinMax.java:
##########
@@ -185,22 +185,22 @@ private Expression addExprMinMaxValues(Expression expr,
ExpressionRewriteContext
&& range.lowerEndpoint().equals(range.upperEndpoint())
&& range.lowerBoundType() == BoundType.CLOSED
&& range.upperBoundType() == BoundType.CLOSED) {
- Expression cmp = new EqualTo(targetExpr, (Literal)
range.lowerEndpoint());
+ Expression cmp = new EqualTo(targetExpr, (Literal)
range.lowerEndpoint()).withInferred(true);
addExprs.add(cmp);
continue;
}
if (range.hasLowerBound()) {
ComparableLiteral literal = range.lowerEndpoint();
Expression cmp = range.lowerBoundType() == BoundType.CLOSED
Review Comment:
Relevant lines:
- AddMinMax.java:188,195-203: generated min/max predicates are marked with
withInferred(true).
- AddMinMax.java:209-223: replaceCmpMinMax() removes duplicate predicates
from the original expression.
- Predicates.java:368-382: compensation collection filters out inferred
predicates.
- PredicatesTest.java:236-267: current test only covers the same
query/view predicate case, not different query/view boundaries.
Suggested GitHub comment:
I am worried about a correctness risk around AddMinMax inferred predicates
and MV compensation.
In AddMinMax.java:188 and 195-203, the generated min/max predicates are
marked as inferred. Then AddMinMax.java:209-223 removes duplicate predicates
from the original expression by expression
equality. Since Expression.equals() does not distinguish inferred vs
non-inferred expressions, an original user-written boundary predicate may be
replaced by an inferred one.
Later, Predicates.java:368-382 filters inferred predicates out when
collecting compensation candidates and query-based view residual predicates.
This means a real query boundary can be
dropped from MV compensation if AddMinMax has replaced it with an inferred
equivalent.
One risky case would be:
MV:
where (id > 10 and id < 20) or (id > 30 and id < 50)
Query:
where (id > 10 and id < 20) or (id > 30 and id < 40)
After AddMinMax, the important boundary difference `id < 40` vs `id < 50`
may only appear in inferred range predicates, while the non-inferred residual
part can look identical. If those
inferred ranges are filtered out during compensation, the rewrite may
incorrectly use the MV without preserving the `id < 40` restriction, returning
rows with `40 <= id < 50`.
The existing test in PredicatesTest.java:236-267 covers the identical
query/view case, but I think we need an end-to-end MV rewrite test where query
and view have different OR min/max
boundaries, and the expected rewritten plan/result still contains the
stricter query boundary.
--
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]
