morrySnow commented on code in PR #64051:
URL: https://github.com/apache/doris/pull/64051#discussion_r3425356029
##########
fe/fe-core/src/test/java/org/apache/doris/nereids/mv/PredicatesTest.java:
##########
@@ -169,58 +179,146 @@ public void
testFinalizeCompensationByResidualKeepsRangeCandidate() {
@Test
public void testResidualCompensateSupportsDnfBranchImplication() {
- PredicateRewriteContext rewriteContext = buildRewriteContext(
- "select id, score from T1 where id = 5 or id > 10",
- "select id, score from T1 where id > 10 or (score = 1 and id =
5)");
-
- PredicateCompensation compensationCandidates =
Predicates.collectCompensationCandidates(
- rewriteContext.queryStructInfo,
- rewriteContext.viewStructInfo,
- rewriteContext.viewToQuerySlotMapping,
- rewriteContext.comparisonResult,
- rewriteContext.queryContext);
- Assertions.assertNotNull(compensationCandidates);
- Assertions.assertTrue(compensationCandidates.getEquals().isEmpty());
- Assertions.assertTrue(compensationCandidates.getRanges().isEmpty());
- Assertions.assertEquals(1,
compensationCandidates.getResiduals().size());
- assertPredicateSqlEquals(compensationCandidates.getResiduals(),
- "OR[(id > 10),AND[(score = 1),(id = 5)]]");
-
- PredicateCompensation finalPredicateCompensation =
compensatePredicates(rewriteContext);
- Assertions.assertNotNull(finalPredicateCompensation);
-
Assertions.assertTrue(finalPredicateCompensation.getEquals().isEmpty());
-
Assertions.assertTrue(finalPredicateCompensation.getRanges().isEmpty());
- Assertions.assertEquals(1,
finalPredicateCompensation.getResiduals().size());
- assertPredicateSqlEquals(finalPredicateCompensation.getResiduals(),
+ assertResidualCompensationSucceeds(
+ "id = 5 or id > 10",
+ "id > 10 or (score = 1 and id = 5)",
"OR[(id > 10),AND[(score = 1),(id = 5)]]");
}
@Test
public void
testResidualCompensateSupportsStrongerRangeInDnfBranchImplication() {
- PredicateRewriteContext rewriteContext = buildRewriteContext(
- "select id, score from T1 where id > 10 or (score = 1 and id =
5)",
- "select id, score from T1 where id > 15 or (score = 1 and id =
5)");
-
- PredicateCompensation compensationCandidates =
Predicates.collectCompensationCandidates(
- rewriteContext.queryStructInfo,
- rewriteContext.viewStructInfo,
- rewriteContext.viewToQuerySlotMapping,
- rewriteContext.comparisonResult,
- rewriteContext.queryContext);
- Assertions.assertNotNull(compensationCandidates);
- Assertions.assertTrue(compensationCandidates.getEquals().isEmpty());
- Assertions.assertTrue(compensationCandidates.getRanges().isEmpty());
- Assertions.assertEquals(1,
compensationCandidates.getResiduals().size());
- assertPredicateSqlEquals(compensationCandidates.getResiduals(),
+ assertResidualCompensationSucceeds(
+ "id > 10 or (score = 1 and id = 5)",
+ "id > 15 or (score = 1 and id = 5)",
"OR[(id > 15),AND[(score = 1),(id = 5)]]");
+ }
- PredicateCompensation finalPredicateCompensation =
compensatePredicates(rewriteContext);
- Assertions.assertNotNull(finalPredicateCompensation);
-
Assertions.assertTrue(finalPredicateCompensation.getEquals().isEmpty());
-
Assertions.assertTrue(finalPredicateCompensation.getRanges().isEmpty());
- Assertions.assertEquals(1,
finalPredicateCompensation.getResiduals().size());
- assertPredicateSqlEquals(finalPredicateCompensation.getResiduals(),
- "OR[(id > 15),AND[(score = 1),(id = 5)]]");
+ @Test
+ public void
testResidualCompensateSupportsComparableLiteralTypesInDnfBranchImplication() {
+ String[][] cases = {
+ {
+ "event_date >= date '2024-01-01' or score = 1",
+ "event_date > date '2024-01-01' or score = 1"
+ },
+ {
+ "event_time <= timestamp '2024-01-02 03:04:05' or
score = 2",
+ "event_time < timestamp '2024-01-02 03:04:05' or score
= 2"
+ },
+ {
+ "amount >= 10.50 or score = 3",
+ "amount = 10.50 or score = 3"
+ },
+ {
+ "tag >= 'm' or score = 4",
+ "tag > 'm' or score = 4"
Review Comment:
**Suggestion (non-blocking):** The case `amount >= 10.50` → `amount = 10.50`
succeeds because `amount = 10.50` is internally converted to range `[10.50,
10.50]`, which implies `[10.50, ∞)`. This is non-obvious at first glance —
consider adding a short inline comment explaining the reasoning, similar to the
boundary-operator test cases.
##########
fe/fe-core/src/test/java/org/apache/doris/nereids/mv/PredicatesTest.java:
##########
@@ -417,6 +515,48 @@ private PredicateCompensation
compensatePredicates(PredicateRewriteContext rewri
rewriteContext.queryContext);
Review Comment:
**Good negative test case.** The extra branch `(score = 1 and id = 7)`
correctly fails because `id = 7` is not covered by any view branch (`id >= 10`
is false, `id IN (5, 6)` is false, `score = 2 AND id <= 3` has the wrong
score). This is a clean demonstration of the safety check rejecting uncovered
DNF branches.
##########
fe/fe-core/src/test/java/org/apache/doris/nereids/mv/PredicatesTest.java:
##########
@@ -417,6 +515,48 @@ private PredicateCompensation
compensatePredicates(PredicateRewriteContext rewri
rewriteContext.queryContext);
}
+ private void assertResidualCompensationSucceeds(String viewPredicate,
String queryPredicate,
+ String... expectedResidualPredicates) {
+ PredicateRewriteContext rewriteContext = buildRewriteContext(
+ SELECT_ALL_TEST_COLUMNS + viewPredicate,
+ SELECT_ALL_TEST_COLUMNS + queryPredicate);
+ PredicateCompensation compensationCandidates =
Predicates.collectCompensationCandidates(
+ rewriteContext.queryStructInfo,
+ rewriteContext.viewStructInfo,
+ rewriteContext.viewToQuerySlotMapping,
+ rewriteContext.comparisonResult,
+ rewriteContext.queryContext);
+ String message = "view predicate: " + viewPredicate + ", query
predicate: " + queryPredicate;
+ Assertions.assertNotNull(compensationCandidates, message);
+ if (expectedResidualPredicates.length != 0) {
+ assertPredicateSqlEquals(compensationCandidates.getResiduals(),
expectedResidualPredicates);
+ }
+
+ PredicateCompensation finalPredicateCompensation =
compensatePredicates(rewriteContext);
+ Assertions.assertNotNull(finalPredicateCompensation, message);
+ if (expectedResidualPredicates.length != 0) {
+
assertPredicateSqlEquals(finalPredicateCompensation.getResiduals(),
expectedResidualPredicates);
+ }
+ }
+
+ private void assertResidualCompensationFails(String viewPredicate, String
queryPredicate) {
+ PredicateRewriteContext rewriteContext = buildRewriteContext(
+ SELECT_ALL_TEST_COLUMNS + viewPredicate,
+ SELECT_ALL_TEST_COLUMNS + queryPredicate);
+ PredicateCompensation compensationCandidates =
Predicates.collectCompensationCandidates(
+ rewriteContext.queryStructInfo,
+ rewriteContext.viewStructInfo,
+ rewriteContext.viewToQuerySlotMapping,
+ rewriteContext.comparisonResult,
+ rewriteContext.queryContext);
+ String message = "view predicate: " + viewPredicate + ", query
predicate: " + queryPredicate;
+ Assertions.assertNotNull(compensationCandidates, message);
+
Assertions.assertFalse(compensationCandidates.getResiduals().isEmpty(),
message);
+
+ PredicateCompensation finalPredicateCompensation =
compensatePredicates(rewriteContext);
+ Assertions.assertNull(finalPredicateCompensation, message);
+ }
+
Review Comment:
**Good defensive check.** The assertion that
`compensationCandidates.getResiduals()` is non-empty before asserting the final
compensation is null guards against accidentally testing a degenerate case
where the view and query predicates are identical (which would trivially
succeed, not fail). One note: if a future failure case had empty residuals but
non-empty equals/ranges, this assertion would incorrectly reject it. For the
current test data this is not an issue.
--
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]
