Skip to content

Fix ClassCastException in UnifiedQueryPlanner.preserveCollation on composite-collation plans#5650

Draft
noCharger wants to merge 1 commit into
opensearch-project:mainfrom
noCharger:fix/unified-query-preserve-collation-composite
Draft

Fix ClassCastException in UnifiedQueryPlanner.preserveCollation on composite-collation plans#5650
noCharger wants to merge 1 commit into
opensearch-project:mainfrom
noCharger:fix/unified-query-preserve-collation-composite

Conversation

@noCharger

Copy link
Copy Markdown
Collaborator

Description

UnifiedQueryPlanner.preserveCollation throws ClassCastException: RelCompositeTrait cannot be cast to RelCollation for any plan whose top RelNode carries more than one collation in its trait set. It surfaces as a 500 Failed to plan query.

Root cause. The method reads the collation via:

RelCollation collation = logical.getTraitSet().getCollation();

RelTraitSet.getCollation() is (RelCollation) getTrait(RelCollationTraitDef.INSTANCE). When a plan derives several collations, Calcite stores them as a single RelCompositeTrait, and the cast fails. A multi-column literal-row plan (for example makeresults format=csv data=..., which lowers to a multi-column LogicalValues) derives such composite collations, so it hits the cast and crashes. A single-column plan derives one collation and survives, which is why this was not caught earlier.

Fix. 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 instead of crashing. Behavior for the single-collation case is unchanged.

Related Issues

N/A

Check List

  • New functionality includes testing.
  • New functionality has been documented.
  • API changes companion pull request created.
  • Commits are signed per the DCO using --signoff.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

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 <lingzhichu.clz@gmail.com>
@github-actions

Copy link
Copy Markdown
Contributor

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🧪 No relevant tests
🔒 No security concerns identified
✅ No TODO sections
🔀 No multiple PR themes
⚡ No major issues detected

@github-actions

Copy link
Copy Markdown
Contributor

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Add null-check for list element

Add a null-check for collations.get(0) before calling getFieldCollations() to
prevent potential NullPointerException. Even though the size check ensures the list
has one element, the element itself could theoretically be null.

api/src/main/java/org/opensearch/sql/api/UnifiedQueryPlanner.java [166-171]

 List<RelCollation> collations =
     logical.getTraitSet().getTraits(RelCollationTraitDef.INSTANCE);
 if (collations != null
     && collations.size() == 1
+    && collations.get(0) != null
     && !collations.get(0).getFieldCollations().isEmpty()) {
   return LogicalSort.create(logical, collations.get(0), null, null);
 }
Suggestion importance[1-10]: 3

__

Why: While adding a null-check for collations.get(0) is technically defensive programming, it's unlikely that the Calcite API would return a list containing a null RelCollation element. The suggestion addresses a theoretical edge case but has minimal practical impact given the API contract.

Low

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant