Skip to content

feat(sql): Support UNION (distinct) for unified SQL - #5741

Merged
dai-chen merged 1 commit into
opensearch-project:mainfrom
dai-chen:fix/ae-union-distinct
Sep 3, 2026
Merged

feat(sql): Support UNION (distinct) for unified SQL#5741
dai-chen merged 1 commit into
opensearch-project:mainfrom
dai-chen:fix/ae-union-distinct

Conversation

@dai-chen

@dai-chen dai-chen commented Sep 2, 2026

Copy link
Copy Markdown
Collaborator

Description

This PR fixed the UNION (distinct) gap for the analytics engine via unified SQL and extended the test-capability mechanism with a per-backend dimension, since set operations are broken on SQL V2 (the JSON response formatter deprecated in 3.0).

Related Issues

Part of #5248

Check List

  • New functionality includes testing.
  • New functionality has been documented.
  • New functionality has javadoc added.
  • New functionality has a user manual doc added.
  • New PPL command checklist all confirmed.
  • API changes companion pull request created.
  • Commits are signed per the DCO using --signoff or -s.
  • Public documentation issue/PR created.

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.

@dai-chen dai-chen self-assigned this Sep 2, 2026
@dai-chen dai-chen added enhancement New feature or request SQL analytic-engine labels Sep 2, 2026
@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

PR Reviewer Guide 🔍

(Review updated until commit 4f9769b)

Here are some key observations to aid the review process:

🧪 PR contains tests
🔒 No security concerns identified
✅ No TODO sections
🔀 No multiple PR themes
⚡ Recommended focus areas for review

Logic Error

The check ctx.ALL().size() == ctx.UNION().size() is incorrect. When parsing UNION ALL UNION ALL, ctx.UNION().size() returns 2 but ctx.ALL().size() also returns 2, so the condition passes. However, when parsing UNION UNION ALL, ctx.UNION().size() is 2 but ctx.ALL().size() is 1, triggering the exception even though this is a valid mixed case that should be rejected. The logic conflates "all UNIONs have ALL" with "no mixing occurred." A correct check would verify that either all or none of the UNIONs have ALL, not that the counts match.

if (ctx.ALL().isEmpty()) {
  return unionDistinct(datasets);
}
if (ctx.ALL().size() == ctx.UNION().size()) {
  return unionAll(datasets);
}
throw new SemanticCheckException(
    "Mixing UNION and UNION ALL in the same query is not supported");

The grammar only accepted UNION ALL and visitUnion hardcoded UNION ALL, so
plain UNION never deduped. Accept optional ALL, carry a distinct flag on the
Union node, and lower it to relBuilder.union(all=!distinct). DataFusion and
the analytics-engine planner already execute LogicalUnion(all=false).

Reject a chain that mixes UNION and UNION ALL: it is left-associative, which
one flat Union cannot represent. Add SetOperationIT and give Capability a
per-backend blacklist, so SET_OPERATION skips on the OpenSearch backend
instead of the analytics engine.

Signed-off-by: Chen Dai <daichen@amazon.com>
@dai-chen
dai-chen force-pushed the fix/ae-union-distinct branch from 97282ac to 4f9769b Compare September 2, 2026 21:48
@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 4f9769b

@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
Simplify conditional logic structure

The logic assumes that if ALL().isEmpty() is false but ALL().size() !=
UNION().size(), it's a mixed case. However, this doesn't account for the case where
ALL().size() > UNION().size(), which is grammatically impossible. Consider
simplifying to an if-else structure for clarity and to avoid unnecessary checks.

api/src/main/java/org/opensearch/sql/api/parser/SqlV2QueryParser.java [159-166]

 if (ctx.ALL().isEmpty()) {
   return unionDistinct(datasets);
+} else if (ctx.ALL().size() == ctx.UNION().size()) {
+  return unionAll(datasets);
+} else {
+  throw new SemanticCheckException(
+      "Mixing UNION and UNION ALL in the same query is not supported");
 }
-if (ctx.ALL().size() == ctx.UNION().size()) {
-  return unionAll(datasets);
-}
-throw new SemanticCheckException(
-    "Mixing UNION and UNION ALL in the same query is not supported");
Suggestion importance[1-10]: 4

__

Why: The suggestion correctly identifies that the logic can be simplified using an if-else structure. While the current code is functionally correct, the suggested refactoring improves readability by making the control flow more explicit. However, the impact is minor as both versions work correctly.

Low

@dai-chen
dai-chen merged commit 96399c5 into opensearch-project:main Sep 3, 2026
41 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants