Skip to content

Commit e9bcf1b

Browse files
committed
[BugFix] Replace inline FQNs with proper imports in dedup regression test (#8)
Add static imports for assertTrue/assertFalse and proper imports for HepPlanner, HepProgram, HepProgramBuilder, PPLSimplifyDedupRule, and LogicalDedup, replacing all inline fully-qualified names in the regression tests added for #7. Pure refactor; no behavior change. Signed-off-by: ryan-gh-bot <ryan-gh-bot@users.noreply.github.com>
1 parent bd848ef commit e9bcf1b

1 file changed

Lines changed: 23 additions & 20 deletions

File tree

ppl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLDedupTest.java

Lines changed: 23 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -5,9 +5,17 @@
55

66
package org.opensearch.sql.ppl.calcite;
77

8+
import static org.junit.Assert.assertFalse;
9+
import static org.junit.Assert.assertTrue;
10+
11+
import org.apache.calcite.plan.hep.HepPlanner;
12+
import org.apache.calcite.plan.hep.HepProgram;
13+
import org.apache.calcite.plan.hep.HepProgramBuilder;
814
import org.apache.calcite.rel.RelNode;
915
import org.apache.calcite.test.CalciteAssert;
1016
import org.junit.Test;
17+
import org.opensearch.sql.calcite.plan.rel.LogicalDedup;
18+
import org.opensearch.sql.calcite.plan.rule.PPLSimplifyDedupRule;
1119

1220
public class CalcitePPLDedupTest extends CalcitePPLAbstractTest {
1321

@@ -356,15 +364,14 @@ public void testSortFieldProjectedAwayBeforeDedup() {
356364

357365
/**
358366
* Regression test for issue #7: when a user {@code where} sits below {@code dedup}, the HEP
359-
* program in {@code CalciteToolsHelper} must still produce a {@link
360-
* org.opensearch.sql.calcite.plan.rel.LogicalDedup}. Before the fix, both rules were registered
361-
* via {@code addRuleCollection}, so {@code FilterMergeRule} could fire ahead of {@code
362-
* PPLSimplifyDedupRule} and merge the user predicate into the bucket-non-null filter; the
363-
* simplify rule's bottom operand then rejected the merged condition (it only accepts pure {@code
364-
* IS NOT NULL}/AND-of-{@code IS NOT NULL}), no {@code LogicalDedup} was produced, and dedup
365-
* pushdown to the OpenSearch storage engine was silently disabled. The fix is to register the two
366-
* rules with separate {@code addRuleInstance} calls in the order simplify-dedup first (to
367-
* fixpoint), then filter-merge.
367+
* program in {@code CalciteToolsHelper} must still produce a {@link LogicalDedup}. Before the
368+
* fix, both rules were registered via {@code addRuleCollection}, so {@code FilterMergeRule} could
369+
* fire ahead of {@code PPLSimplifyDedupRule} and merge the user predicate into the
370+
* bucket-non-null filter; the simplify rule's bottom operand then rejected the merged condition
371+
* (it only accepts pure {@code IS NOT NULL}/AND-of-{@code IS NOT NULL}), no {@code LogicalDedup}
372+
* was produced, and dedup pushdown to the OpenSearch storage engine was silently disabled. The
373+
* fix is to register the two rules with separate {@code addRuleInstance} calls in the order
374+
* simplify-dedup first (to fixpoint), then filter-merge.
368375
*/
369376
@Test
370377
public void testWhereThenDedupProducesLogicalDedup() {
@@ -375,13 +382,13 @@ public void testWhereThenDedupProducesLogicalDedup() {
375382
String ppl = "source=EMP | where SAL > 1000 | dedup 1 DEPTNO | fields DEPTNO";
376383
RelNode optimized = getRelNodeAfterCalciteHep(ppl);
377384
String optimizedPlan = optimized.explain();
378-
org.junit.Assert.assertTrue(
385+
assertTrue(
379386
"where + dedup must produce a LogicalDedup so OpenSearch DedupPushdownRule can match;"
380387
+ " actual plan was:\n"
381388
+ optimizedPlan,
382389
optimizedPlan.contains("LogicalDedup"));
383390
// The window-form leftover would indicate the simplify rule did not fire — assert it is gone.
384-
org.junit.Assert.assertFalse(
391+
assertFalse(
385392
"ROW_NUMBER window must be consumed by PPLSimplifyDedupRule when where + dedup are"
386393
+ " combined; actual plan was:\n"
387394
+ optimizedPlan,
@@ -403,25 +410,21 @@ public void testFilterMergeBeforeSimplifyDedupBreaksPattern() {
403410
// getRelNode already runs FilterMergeRule on the raw plan, simulating the pathological
404411
// schedule where FilterMergeRule fires before PPLSimplifyDedupRule.
405412
RelNode mergedFirst = getRelNode(ppl);
406-
org.apache.calcite.plan.hep.HepProgram simplifyOnly =
407-
new org.apache.calcite.plan.hep.HepProgramBuilder()
408-
.addRuleInstance(
409-
org.opensearch.sql.calcite.plan.rule.PPLSimplifyDedupRule.DEDUP_SIMPLIFY_RULE)
410-
.build();
411-
org.apache.calcite.plan.hep.HepPlanner planner =
412-
new org.apache.calcite.plan.hep.HepPlanner(simplifyOnly);
413+
HepProgram simplifyOnly =
414+
new HepProgramBuilder().addRuleInstance(PPLSimplifyDedupRule.DEDUP_SIMPLIFY_RULE).build();
415+
HepPlanner planner = new HepPlanner(simplifyOnly);
413416
planner.setRoot(mergedFirst);
414417
RelNode result = planner.findBestExp();
415418
String plan = result.explain();
416-
org.junit.Assert.assertFalse(
419+
assertFalse(
417420
"If FilterMergeRule runs before PPLSimplifyDedupRule, the simplify rule must NOT recover"
418421
+ " — the merged AND(IS_NOT_NULL, user_predicate) filter fails the bucket-non-null"
419422
+ " predicate. This documents why the production HEP program enforces ordering via"
420423
+ " separate addRuleInstance calls (PPLSimplifyDedupRule first, then FilterMergeRule)."
421424
+ " Actual plan was:\n"
422425
+ plan,
423426
plan.contains("LogicalDedup"));
424-
org.junit.Assert.assertTrue(
427+
assertTrue(
425428
"Plan should still contain ROW_NUMBER window form when simplify fails. Actual plan was:\n"
426429
+ plan,
427430
plan.contains("ROW_NUMBER"));

0 commit comments

Comments
 (0)