Skip to content

Commit 68601ff

Browse files
committed
Fix explain unit test to verify actual logical plan content
Use a real Calcite LogicalValues node instead of a mock so RelOptUtil.toString() produces a deterministic plan. Assert the exact expected logical plan string instead of just checking non-null. Signed-off-by: Kai Huang <kaihuang@amazon.com> Signed-off-by: Kai Huang <ahkcs@amazon.com>
1 parent 64709d9 commit 68601ff

File tree

1 file changed

+33
-31
lines changed

1 file changed

+33
-31
lines changed

core/src/test/java/org/opensearch/sql/executor/analytics/AnalyticsExecutionEngineTest.java

Lines changed: 33 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77

88
import static org.junit.jupiter.api.Assertions.assertEquals;
99
import static org.junit.jupiter.api.Assertions.assertNotNull;
10+
import static org.junit.jupiter.api.Assertions.assertNull;
1011
import static org.junit.jupiter.api.Assertions.assertTrue;
1112
import static org.mockito.Mockito.mock;
1213
import static org.mockito.Mockito.when;
@@ -249,43 +250,29 @@ void physicalPlanExplain_callsOnFailure() {
249250

250251
@Test
251252
void explainRelNode_returnsLogicalPlan() {
252-
RelNode relNode = mockRelNode("name", SqlTypeName.VARCHAR, "age", SqlTypeName.INTEGER);
253+
// Build a real RelNode so RelOptUtil.toString() produces a deterministic plan
254+
org.apache.calcite.tools.FrameworkConfig config =
255+
org.apache.calcite.tools.Frameworks.newConfigBuilder().build();
256+
org.apache.calcite.tools.RelBuilder builder =
257+
org.apache.calcite.tools.RelBuilder.create(config);
258+
RelNode relNode = builder.values(new String[] {"name", "age"}, "Alice", 30, "Bob", 25).build();
253259

254260
AtomicReference<ExplainResponse> ref = new AtomicReference<>();
255-
AtomicReference<Exception> errorRef = new AtomicReference<>();
256261
engine.explain(
257262
relNode,
258263
org.opensearch.sql.ast.statement.ExplainMode.STANDARD,
259264
mockContext,
260-
new ResponseListener<ExplainResponse>() {
261-
@Override
262-
public void onResponse(ExplainResponse response) {
263-
ref.set(response);
264-
}
265-
266-
@Override
267-
public void onFailure(Exception e) {
268-
errorRef.set(e);
269-
}
270-
});
271-
272-
// RelOptUtil.toString on a mock may throw or succeed depending on mock setup.
273-
// Accept either a successful response or a caught failure.
274-
assertTrue(
275-
ref.get() != null || errorRef.get() != null,
276-
"Either onResponse or onFailure should have been called");
277-
if (ref.get() != null) {
278-
assertNotNull(ref.get().getCalcite(), "Explain should have calcite field");
279-
assertNotNull(ref.get().getCalcite().getLogical(), "Explain should have logical plan");
280-
System.out.println(
281-
"\n--- explainRelNode_returnsLogicalPlan ---\nLogical: "
282-
+ ref.get().getCalcite().getLogical()
283-
+ "Physical: "
284-
+ ref.get().getCalcite().getPhysical()
285-
+ "\nExtended: "
286-
+ ref.get().getCalcite().getExtended()
287-
+ "\n--- End ---");
288-
}
265+
captureExplainListener(ref));
266+
267+
assertNotNull(ref.get(), "ExplainResponse should not be null");
268+
String expectedLogical = "LogicalValues(tuples=[[{ 'Alice', 30 }, { 'Bob', 25 }]])\n";
269+
assertEquals(expectedLogical, ref.get().getCalcite().getLogical(), "Logical plan mismatch");
270+
assertNull(ref.get().getCalcite().getPhysical(), "Physical plan should be null");
271+
assertNull(ref.get().getCalcite().getExtended(), "Extended plan should be null");
272+
System.out.println(
273+
"\n--- explainRelNode_returnsLogicalPlan ---\n"
274+
+ ref.get().getCalcite().getLogical()
275+
+ "--- End ---");
289276
}
290277

291278
@Test
@@ -325,6 +312,21 @@ private QueryResponse executeAndCapture(RelNode relNode) {
325312
return ref.get();
326313
}
327314

315+
private ResponseListener<ExplainResponse> captureExplainListener(
316+
AtomicReference<ExplainResponse> ref) {
317+
return new ResponseListener<ExplainResponse>() {
318+
@Override
319+
public void onResponse(ExplainResponse response) {
320+
ref.set(response);
321+
}
322+
323+
@Override
324+
public void onFailure(Exception e) {
325+
throw new AssertionError("Unexpected failure", e);
326+
}
327+
};
328+
}
329+
328330
private Exception executeAndCaptureError(RelNode relNode) {
329331
AtomicReference<Exception> ref = new AtomicReference<>();
330332
engine.execute(

0 commit comments

Comments
 (0)