From 3f39114faa1fd1952ab118a239533e1e828f680d Mon Sep 17 00:00:00 2001 From: Louis Chu Date: Fri, 24 Jul 2026 01:38:22 +0800 Subject: [PATCH 1/2] Fix ClassCastException in preserveCollation on composite-collation plans UnifiedQueryPlanner.preserveCollation reads the collation via RelTraitSet.getCollation(), which casts to a single RelCollation. When a plan derives several collations (Calcite stores them as a RelCompositeTrait) the cast throws ClassCastException and the query fails to plan. A multi-column literal-row plan (e.g. makeresults data=, which lowers to a multi-column LogicalValues) derives such composite collations and hits this; a single-column plan derives one collation and survives. Read the collation through the composite-safe getTraits(RelCollationTraitDef.INSTANCE) and materialize a LogicalSort only when there is exactly one genuine non-empty sort collation. A composite (several incidental derived collations) is not a user sort, so the plan is left unwrapped. Single-collation behavior is unchanged. Signed-off-by: Louis Chu --- .../sql/api/UnifiedQueryPlanner.java | 20 +++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/api/src/main/java/org/opensearch/sql/api/UnifiedQueryPlanner.java b/api/src/main/java/org/opensearch/sql/api/UnifiedQueryPlanner.java index 9440833503f..b5622f98a1e 100644 --- a/api/src/main/java/org/opensearch/sql/api/UnifiedQueryPlanner.java +++ b/api/src/main/java/org/opensearch/sql/api/UnifiedQueryPlanner.java @@ -7,9 +7,10 @@ import static org.opensearch.sql.monitor.profile.MetricName.ANALYZE; +import java.util.List; import lombok.RequiredArgsConstructor; import org.apache.calcite.rel.RelCollation; -import org.apache.calcite.rel.RelCollations; +import org.apache.calcite.rel.RelCollationTraitDef; import org.apache.calcite.rel.RelNode; import org.apache.calcite.rel.RelRoot; import org.apache.calcite.rel.core.Sort; @@ -154,9 +155,20 @@ public RelNode plan(String query) { } private RelNode preserveCollation(RelNode logical) { - RelCollation collation = logical.getTraitSet().getCollation(); - if (!(logical instanceof Sort) && collation != RelCollations.EMPTY) { - return LogicalSort.create(logical, collation, null, null); + if (logical instanceof Sort) { + return logical; + } + // Only a genuine single-sort collation is materialized. A literal-row plan (makeresults + // data=) carries several incidental derived collations kept as a RelCompositeTrait; those + // are not a user sort, so getTraits returns more than one and we leave the plan unwrapped. + // RelTraitSet.getCollation() would instead cast the composite to a single RelCollation and + // throw. + List collations = + logical.getTraitSet().getTraits(RelCollationTraitDef.INSTANCE); + if (collations != null + && collations.size() == 1 + && !collations.get(0).getFieldCollations().isEmpty()) { + return LogicalSort.create(logical, collations.get(0), null, null); } return logical; } From cb87a7eb24a7d5bf5a949121b9c09e07fdc47830 Mon Sep 17 00:00:00 2001 From: Louis Chu Date: Fri, 31 Jul 2026 00:14:37 +0800 Subject: [PATCH 2/2] Add tests for preserveCollation composite-collation fix; drop comment Cover UnifiedQueryPlanner#preserveCollation with tests and remove the now-redundant inline comment (the rationale lives in the commit/PR). Unit tests (UnifiedQueryPlannerTest) exercise all four branches: - an existing top-level Sort is returned unchanged (no double-wrap); - a composite collation is left unwrapped instead of crashing (the regression: `sort age | eval x = age` makes x mirror age, so the top LogicalProject derives multiple collations stored as a RelCompositeTrait; the old getCollation() read threw "Trait index N has multiple values", surfacing as a 500); - a single genuine collation is materialized as a LogicalSort; - an unsorted plan is returned unwrapped. Execution coverage: UnifiedQueryCompilerTest compiles and runs the previously-crashing plan in-memory, and UnifiedQueryOpenSearchIT runs the same sort+eval-copy shape end to end against a real OpenSearch index. The composite-collation tests were confirmed to fail against the pre-fix implementation. Signed-off-by: Louis Chu --- .../sql/api/UnifiedQueryPlanner.java | 5 --- .../sql/api/UnifiedQueryPlannerTest.java | 37 +++++++++++++++++++ .../compiler/UnifiedQueryCompilerTest.java | 21 +++++++++++ .../sql/api/UnifiedQueryOpenSearchIT.java | 24 ++++++++++++ 4 files changed, 82 insertions(+), 5 deletions(-) diff --git a/api/src/main/java/org/opensearch/sql/api/UnifiedQueryPlanner.java b/api/src/main/java/org/opensearch/sql/api/UnifiedQueryPlanner.java index b5622f98a1e..bc86ac430de 100644 --- a/api/src/main/java/org/opensearch/sql/api/UnifiedQueryPlanner.java +++ b/api/src/main/java/org/opensearch/sql/api/UnifiedQueryPlanner.java @@ -158,11 +158,6 @@ private RelNode preserveCollation(RelNode logical) { if (logical instanceof Sort) { return logical; } - // Only a genuine single-sort collation is materialized. A literal-row plan (makeresults - // data=) carries several incidental derived collations kept as a RelCompositeTrait; those - // are not a user sort, so getTraits returns more than one and we leave the plan unwrapped. - // RelTraitSet.getCollation() would instead cast the composite to a single RelCollation and - // throw. List collations = logical.getTraitSet().getTraits(RelCollationTraitDef.INSTANCE); if (collations != null diff --git a/api/src/test/java/org/opensearch/sql/api/UnifiedQueryPlannerTest.java b/api/src/test/java/org/opensearch/sql/api/UnifiedQueryPlannerTest.java index 008121e8377..085171b7764 100644 --- a/api/src/test/java/org/opensearch/sql/api/UnifiedQueryPlannerTest.java +++ b/api/src/test/java/org/opensearch/sql/api/UnifiedQueryPlannerTest.java @@ -5,11 +5,14 @@ package org.opensearch.sql.api; +import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertThrows; +import static org.junit.Assert.assertTrue; import java.util.Map; import org.apache.calcite.rel.RelNode; +import org.apache.calcite.rel.core.Sort; import org.apache.calcite.runtime.CalciteException; import org.apache.calcite.schema.Schema; import org.apache.calcite.schema.impl.AbstractSchema; @@ -198,4 +201,38 @@ public void testPPLPatternsPicksUpDefaults() { .assertPlanContains("REGEXP_REPLACE") .assertFields("id", "name", "age", "department", "patterns_field"); } + + @Test + public void preserveCollationReturnsExistingSortUnchanged() { + var assertion = givenQuery("source = catalog.employees | sort age"); + assertTrue("top node should be the user's Sort", assertion.plan() instanceof Sort); + assertion.assertPlan( + "LogicalSort(sort0=[$2], dir0=[ASC-nulls-first])\n" + + " LogicalTableScan(table=[[catalog, employees]])\n"); + } + + @Test + public void preserveCollationLeavesCompositeCollationUnwrapped() { + var assertion = givenQuery("source = catalog.employees | sort age | eval x = age"); + assertFalse( + "composite-collation plan must not be re-wrapped in a Sort", + assertion.plan() instanceof Sort); + assertion.assertFields("id", "name", "age", "department", "x"); + } + + @Test + public void preserveCollationMaterializesSingleDerivedCollation() { + givenQuery("makeresults format=csv data='name\nJohn\nSarah'") + .assertPlan( + "LogicalSort(sort0=[$0], dir0=[ASC])\n" + + " LogicalProject(name=[CAST($0):VARCHAR NOT NULL])\n" + + " LogicalValues(tuples=[[{ 'John' }, { 'Sarah' }]])\n"); + } + + @Test + public void preserveCollationLeavesUnsortedPlanUnwrapped() { + var assertion = givenQuery("source = catalog.employees"); + assertFalse("unsorted plan must not be wrapped in a Sort", assertion.plan() instanceof Sort); + assertion.assertPlan("LogicalTableScan(table=[[catalog, employees]])\n"); + } } diff --git a/api/src/test/java/org/opensearch/sql/api/compiler/UnifiedQueryCompilerTest.java b/api/src/test/java/org/opensearch/sql/api/compiler/UnifiedQueryCompilerTest.java index f344969cdc3..9ca25f1e523 100644 --- a/api/src/test/java/org/opensearch/sql/api/compiler/UnifiedQueryCompilerTest.java +++ b/api/src/test/java/org/opensearch/sql/api/compiler/UnifiedQueryCompilerTest.java @@ -57,6 +57,27 @@ public void testComplexQuery() throws Exception { } } + @Test + public void testCompositeCollationQueryExecutes() throws Exception { + RelNode plan = planner.plan("source = catalog.employees | sort age | eval x = age"); + try (PreparedStatement statement = compiler.compile(plan)) { + ResultSet resultSet = statement.executeQuery(); + + verify(resultSet) + .expectSchema( + col("id", INTEGER), + col("name", VARCHAR), + col("age", INTEGER), + col("department", VARCHAR), + col("x", INTEGER)) + .expectData( + row(1, "Alice", 25, "Engineering", 25), + row(2, "Bob", 35, "Sales", 35), + row(3, "Charlie", 45, "Engineering", 45), + row(4, "Diana", 28, "Marketing", 28)); + } + } + @Test(expected = IllegalStateException.class) public void testCompileFailure() { RelNode mockPlan = Mockito.mock(RelNode.class); diff --git a/integ-test/src/test/java/org/opensearch/sql/api/UnifiedQueryOpenSearchIT.java b/integ-test/src/test/java/org/opensearch/sql/api/UnifiedQueryOpenSearchIT.java index 7f7d31790f1..df7557976f0 100644 --- a/integ-test/src/test/java/org/opensearch/sql/api/UnifiedQueryOpenSearchIT.java +++ b/integ-test/src/test/java/org/opensearch/sql/api/UnifiedQueryOpenSearchIT.java @@ -104,6 +104,30 @@ public void testMultiplePPLQueryExecutionWithSameContext() throws Exception { } } + @Test + public void testSortThenEvalCopyExecutesEndToEnd() throws Exception { + String pplQuery = + String.format( + "source = opensearch.%s | sort age | eval age_copy = age" + + " | fields firstname, age, age_copy", + TEST_INDEX_ACCOUNT); + + RelNode plan = planner.plan(pplQuery); + try (PreparedStatement statement = compiler.compile(plan)) { + ResultSet resultSet = statement.executeQuery(); + + verify(resultSet) + .expectSchema(col("firstname", VARCHAR), col("age", BIGINT), col("age_copy", BIGINT)); + + int rows = 0; + while (resultSet.next()) { + rows++; + assertEquals("age_copy must mirror age", resultSet.getObject(2), resultSet.getObject(3)); + } + assertTrue("expected at least one row", rows > 0); + } + } + /** * Creates a dynamic schema that creates OpenSearchIndex on-demand for any table name. This allows * querying any index without pre-registering it.