github-actions[bot] commented on code in PR #61178:
URL: https://github.com/apache/doris/pull/61178#discussion_r2910274158
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/exploration/mv/Predicates.java:
##########
@@ -128,121 +128,105 @@ public static Map<Expression, ExpressionInfo>
compensateCouldNotPullUpPredicates
}
/**
- * compensate equivalence predicates
+ * Compensate equivalence predicates based on equivalence classes.
+ * Collects uncovered equivalence predicates into uncoveredEquals for
residual compensation.
*/
public static Map<Expression, ExpressionInfo>
compensateEquivalence(StructInfo queryStructInfo,
StructInfo viewStructInfo,
SlotMapping viewToQuerySlotMapping,
- ComparisonResult comparisonResult) {
+ ComparisonResult comparisonResult,
+ Set<Expression> uncoveredEquals) {
EquivalenceClass queryEquivalenceClass =
queryStructInfo.getEquivalenceClass();
EquivalenceClass viewEquivalenceClass =
viewStructInfo.getEquivalenceClass();
Map<SlotReference, SlotReference> viewToQuerySlotMap =
viewToQuerySlotMapping.toSlotReferenceMap();
EquivalenceClass viewEquivalenceClassQueryBased =
viewEquivalenceClass.permute(viewToQuerySlotMap);
if (viewEquivalenceClassQueryBased == null) {
return null;
}
- final Map<Expression, ExpressionInfo> equalCompensateConjunctions =
new HashMap<>();
if (queryEquivalenceClass.isEmpty() && viewEquivalenceClass.isEmpty())
{
return ImmutableMap.of();
}
- if (queryEquivalenceClass.isEmpty() &&
!viewEquivalenceClass.isEmpty()) {
- return null;
- }
EquivalenceClassMapping queryToViewEquivalenceMapping =
EquivalenceClassMapping.generate(queryEquivalenceClass,
viewEquivalenceClassQueryBased);
- // can not map all target equivalence class, can not compensate
if (queryToViewEquivalenceMapping.getEquivalenceClassSetMap().size()
< viewEquivalenceClass.getEquivalenceSetList().size()) {
return null;
}
- // do equal compensate
+ Map<Expression, ExpressionInfo> compensations = new HashMap<>();
Set<List<SlotReference>> mappedQueryEquivalenceSet =
queryToViewEquivalenceMapping.getEquivalenceClassSetMap().keySet();
+ // Process each query equivalence class:
+ // 1) If it is not mapped, view cannot cover this equal-set, so defer
it to residual compensation.
+ // 2) If it is mapped, only compensate the extra equal constraints
introduced by query.
for (List<SlotReference> queryEquivalenceSet :
queryEquivalenceClass.getEquivalenceSetList()) {
- // compensate the equivalence in query but not in view
if (!mappedQueryEquivalenceSet.contains(queryEquivalenceSet)) {
- Iterator<SlotReference> iterator =
queryEquivalenceSet.iterator();
- SlotReference first = iterator.next();
- while (iterator.hasNext()) {
- Expression equals = new EqualTo(first, iterator.next());
- if (equals.anyMatch(AggregateFunction.class::isInstance)) {
- return null;
- }
- equalCompensateConjunctions.put(equals,
ExpressionInfo.EMPTY);
- }
+ // Uncovered class: use the first slot as anchor and generate
a minimal equality set.
+ SlotReference first = queryEquivalenceSet.get(0);
+ queryEquivalenceSet.stream()
+ .skip(1)
+ .map(slot -> new EqualTo(first, slot))
+ .forEach(uncoveredEquals::add);
} else {
Review Comment:
**Dropped `AggregateFunction` check for uncovered equals.**
The old code checked `if
(equals.anyMatch(AggregateFunction.class::isInstance)) return null;` for each
generated equality before adding to `equalCompensateConjunctions`. The new code
generates `EqualTo` expressions and adds them directly to `uncoveredEquals`
without this check.
This is safe because: (a) equivalence class slots are `SlotReference`
objects, not aggregate functions, so the check would never trigger, and (b)
these expressions will be checked later in `compensateResidualPredicate` (line
303). A brief comment noting this would be helpful for future readers.
##########
fe/fe-core/src/test/java/org/apache/doris/nereids/mv/PredicatesTest.java:
##########
@@ -177,8 +182,68 @@ public void
testCompensateCouldNotPullUpPredicatesSuccess() {
Map<Expression, ExpressionInfo> compensateRangePredicates =
Predicates.compensateRangePredicate(
queryStructInfo, mvStructInfo, mvToQuerySlotMapping,
comparisonResult,
- queryContext);
+ queryContext, new HashSet<>());
Assertions.assertNotNull(compensateRangePredicates);
Assertions.assertEquals(1, compensateRangePredicates.size());
+ String rangeSql =
compensateRangePredicates.keySet().iterator().next().toSql();
+ Assertions.assertEquals("(score > 15)", rangeSql);
+ }
+
+ @Test
+ public void testResidualCompensateWithUncoveredRangeAsExtraResidual() {
Review Comment:
**Test coverage gaps:**
1. **No negative test cases for `computeResidualCompensation`**: Missing
tests for scenarios where MV rewrite should fail, e.g.:
- Query OR is a superset of MV OR (MV: `id=5 OR id=2`, Query: `id=5 OR
id=2 OR id>10`)
- MV has residual predicates but query has none
2. **No test for `compensateEquivalence` with `uncoveredEquals`**: The
`uncoveredEquals` output parameter is introduced but has zero test coverage.
3. **No end-to-end regression test**: Per the project's testing standards,
all kernel features must have corresponding regression tests under
`regression-test/`. This OR-subset compensation feature has no regression test.
4. **No test for exact-match OR case**: MV and query have identical OR
predicates — compensation should remove the query residual (empty compensation).
5. **No test for multiple OR clauses**: e.g., MV has `(a=1 OR a=2) AND (b=3
OR b=4)` and query has a subset of one or both.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/exploration/mv/Predicates.java:
##########
@@ -128,121 +128,105 @@ public static Map<Expression, ExpressionInfo>
compensateCouldNotPullUpPredicates
}
/**
- * compensate equivalence predicates
+ * Compensate equivalence predicates based on equivalence classes.
+ * Collects uncovered equivalence predicates into uncoveredEquals for
residual compensation.
*/
public static Map<Expression, ExpressionInfo>
compensateEquivalence(StructInfo queryStructInfo,
StructInfo viewStructInfo,
SlotMapping viewToQuerySlotMapping,
- ComparisonResult comparisonResult) {
+ ComparisonResult comparisonResult,
+ Set<Expression> uncoveredEquals) {
EquivalenceClass queryEquivalenceClass =
queryStructInfo.getEquivalenceClass();
EquivalenceClass viewEquivalenceClass =
viewStructInfo.getEquivalenceClass();
Map<SlotReference, SlotReference> viewToQuerySlotMap =
viewToQuerySlotMapping.toSlotReferenceMap();
EquivalenceClass viewEquivalenceClassQueryBased =
viewEquivalenceClass.permute(viewToQuerySlotMap);
if (viewEquivalenceClassQueryBased == null) {
return null;
}
- final Map<Expression, ExpressionInfo> equalCompensateConjunctions =
new HashMap<>();
if (queryEquivalenceClass.isEmpty() && viewEquivalenceClass.isEmpty())
{
return ImmutableMap.of();
}
- if (queryEquivalenceClass.isEmpty() &&
!viewEquivalenceClass.isEmpty()) {
- return null;
- }
EquivalenceClassMapping queryToViewEquivalenceMapping =
EquivalenceClassMapping.generate(queryEquivalenceClass,
viewEquivalenceClassQueryBased);
- // can not map all target equivalence class, can not compensate
if (queryToViewEquivalenceMapping.getEquivalenceClassSetMap().size()
< viewEquivalenceClass.getEquivalenceSetList().size()) {
return null;
}
- // do equal compensate
+ Map<Expression, ExpressionInfo> compensations = new HashMap<>();
Set<List<SlotReference>> mappedQueryEquivalenceSet =
queryToViewEquivalenceMapping.getEquivalenceClassSetMap().keySet();
+ // Process each query equivalence class:
+ // 1) If it is not mapped, view cannot cover this equal-set, so defer
it to residual compensation.
+ // 2) If it is mapped, only compensate the extra equal constraints
introduced by query.
for (List<SlotReference> queryEquivalenceSet :
queryEquivalenceClass.getEquivalenceSetList()) {
- // compensate the equivalence in query but not in view
if (!mappedQueryEquivalenceSet.contains(queryEquivalenceSet)) {
- Iterator<SlotReference> iterator =
queryEquivalenceSet.iterator();
- SlotReference first = iterator.next();
- while (iterator.hasNext()) {
- Expression equals = new EqualTo(first, iterator.next());
- if (equals.anyMatch(AggregateFunction.class::isInstance)) {
- return null;
- }
- equalCompensateConjunctions.put(equals,
ExpressionInfo.EMPTY);
- }
+ // Uncovered class: use the first slot as anchor and generate
a minimal equality set.
+ SlotReference first = queryEquivalenceSet.get(0);
+ queryEquivalenceSet.stream()
+ .skip(1)
+ .map(slot -> new EqualTo(first, slot))
+ .forEach(uncoveredEquals::add);
} else {
- // compensate the equivalence both in query and view, but
query has more equivalence
+ // Partially covered: avoid duplicating view equalities, only
add query extras.
List<SlotReference> viewEquivalenceSet =
queryToViewEquivalenceMapping.getEquivalenceClassSetMap().get(queryEquivalenceSet);
- List<SlotReference> copiedQueryEquivalenceSet = new
ArrayList<>(queryEquivalenceSet);
- copiedQueryEquivalenceSet.removeAll(viewEquivalenceSet);
- SlotReference first = viewEquivalenceSet.iterator().next();
- for (SlotReference slotReference : copiedQueryEquivalenceSet) {
- Expression equals = new EqualTo(first, slotReference);
+ List<SlotReference> queryExtraSlots = new
ArrayList<>(queryEquivalenceSet);
+ queryExtraSlots.removeAll(viewEquivalenceSet);
+
+ // Build compensation predicates by pairing a view base slot
with each extra query slot.
+ SlotReference firstViewSlot = viewEquivalenceSet.get(0);
+ for (SlotReference extraSlot : queryExtraSlots) {
+ Expression equals = new EqualTo(firstViewSlot, extraSlot);
if (equals.anyMatch(AggregateFunction.class::isInstance)) {
return null;
}
- equalCompensateConjunctions.put(equals,
ExpressionInfo.EMPTY);
+ compensations.put(equals, ExpressionInfo.EMPTY);
}
}
}
- return equalCompensateConjunctions;
+ return ImmutableMap.copyOf(compensations);
}
/**
- * compensate range predicates
+ * Compensate range predicates.
+ * Collects uncovered range predicates into uncoveredRanges for residual
compensation.
*/
public static Map<Expression, ExpressionInfo>
compensateRangePredicate(StructInfo queryStructInfo,
StructInfo viewStructInfo,
SlotMapping viewToQuerySlotMapping,
ComparisonResult comparisonResult,
- CascadesContext cascadesContext) {
+ CascadesContext cascadesContext,
+ Set<Expression> uncoveredRanges) {
SplitPredicate querySplitPredicate =
queryStructInfo.getSplitPredicate();
SplitPredicate viewSplitPredicate = viewStructInfo.getSplitPredicate();
- Set<Expression> viewRangeQueryBasedSet = new HashSet<>();
- for (Expression viewExpression :
viewSplitPredicate.getRangePredicateMap().keySet()) {
- viewRangeQueryBasedSet.add(
- ExpressionUtils.replace(viewExpression,
viewToQuerySlotMapping.toSlotReferenceMap()));
- }
- viewRangeQueryBasedSet.remove(BooleanLiteral.TRUE);
+ Map<SlotReference, SlotReference> slotMap =
viewToQuerySlotMapping.toSlotReferenceMap();
+ Set<Expression> viewRangeSetQueryBased =
viewSplitPredicate.getRangePredicateMap().keySet().stream()
+ .filter(expr -> !ExpressionUtils.isInferred(expr))
+ .map(expr -> ExpressionUtils.replace(expr, slotMap))
+ .filter(expr -> expr != BooleanLiteral.TRUE)
+ .collect(ImmutableSet.toImmutableSet());
- Set<Expression> queryRangeSet =
querySplitPredicate.getRangePredicateMap().keySet();
- queryRangeSet.remove(BooleanLiteral.TRUE);
+ Set<Expression> queryRangeSet =
querySplitPredicate.getRangePredicateMap().keySet().stream()
+ .filter(expr -> !ExpressionUtils.isInferred(expr))
+ .collect(ImmutableSet.toImmutableSet());
Set<Expression> differentExpressions = new HashSet<>();
- Sets.difference(queryRangeSet,
viewRangeQueryBasedSet).copyInto(differentExpressions);
- Sets.difference(viewRangeQueryBasedSet,
queryRangeSet).copyInto(differentExpressions);
- // the range predicate in query and view is same, don't need to
compensate
+ Sets.difference(queryRangeSet,
viewRangeSetQueryBased).copyInto(differentExpressions);
+ Sets.difference(viewRangeSetQueryBased,
queryRangeSet).copyInto(differentExpressions);
if (differentExpressions.isEmpty()) {
return ImmutableMap.of();
}
- // try to normalize the different expressions
- Set<Expression> normalizedExpressions =
- normalizeExpression(ExpressionUtils.and(differentExpressions),
cascadesContext);
+ Set<Expression> normalizedExpressions = new HashSet<>(
+ normalizeExpression(ExpressionUtils.and(differentExpressions),
cascadesContext));
+ // Avoid entering subsequent containsAll/uncoveredRanges; otherwise
residual compensation may fail.
+ normalizedExpressions.remove(BooleanLiteral.TRUE);
if (!queryRangeSet.containsAll(normalizedExpressions)) {
- // normalized expressions is not in query, can not compensate
return null;
}
- Map<Expression, ExpressionInfo> normalizedExpressionsWithLiteral = new
HashMap<>();
- for (Expression expression : normalizedExpressions) {
- Set<Literal> literalSet = expression.collect(expressionTreeNode ->
expressionTreeNode instanceof Literal);
- if (!(expression instanceof ComparisonPredicate)
- || (expression instanceof GreaterThan || expression
instanceof LessThanEqual)
- || literalSet.size() != 1) {
- if (expression.anyMatch(AggregateFunction.class::isInstance)) {
- return null;
- }
- normalizedExpressionsWithLiteral.put(expression,
ExpressionInfo.EMPTY);
- continue;
- }
- if (expression.anyMatch(AggregateFunction.class::isInstance)) {
- return null;
- }
- normalizedExpressionsWithLiteral.put(expression, new
ExpressionInfo(literalSet.iterator().next()));
- }
- return normalizedExpressionsWithLiteral;
+ uncoveredRanges.addAll(normalizedExpressions);
+ return buildRangeExpressionInfo(normalizedExpressions);
Review Comment:
**Design flaw: double-counting of range compensation predicates.**
`normalizedExpressions` is both returned as the range compensation map (via
`buildRangeExpressionInfo`) AND added to `uncoveredRanges`. These uncovered
ranges flow into `compensateResidualPredicate` as `extraQueryResiduals`, where
they can survive into the residual compensation map. When
`SplitPredicate.toList()` concatenates all three maps, the same expression can
appear twice.
This is not a correctness bug because `Sets.newLinkedHashSet(...)` in
`AbstractMaterializedViewRule` (line 289) deduplicates before creating
`LogicalFilter`. However, it causes wasted work in `rewriteExpression` and is
fragile — it relies on `Expression.equals()` being consistent across different
code paths.
Consider either:
1. Only adding to `uncoveredRanges` and returning an empty map when the
expressions are fully deferred to residual compensation, or
2. Not adding range expressions that are already in the range compensation
to `uncoveredRanges`, or
3. Adding a comment explaining the intentional duplication and reliance on
downstream dedup.
--
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]
