[Enhancement] Add error handling for known limitation of sql `JOIN` by RyanL1997 · Pull Request #4344 · opensearch-project/sql

RyanL1997 · 2025-09-22T00:18:41Z

Signed-off-by: Jialiang Liang jiallian@amazon.com

Description

Better error handling for known limitation of sql JOIN

example:

❯   curl -X POST "localhost:9200/_plugins/_sql" -H 'Content-Type: application/json' -d'
  {
    "query": "SELECT SUM(a.balance) FROM accounts a JOIN accounts b ON a.account_number = b.account_number"
  }'

{
  "error": {
    "reason": "Invalid SQL query",
    "details": "JOIN queries do not support aggregations on the joined result. For more information, see https://docs.opensearch.org/3.2/search-plugins/sql/limitation/#join-does-not-support-aggregations-on-the-joined-result",
    "type": "SqlParseException"
  },
  "status": 400
}%

Related Issues

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.

Swiddis

A few suggestions -- I think this could be designed better if changing JoinSelect doesn't cause cascading breakage, otherwise this approach is fine.

legacy/src/main/java/org/opensearch/sql/legacy/parser/SqlParser.java

legacy/src/test/java/org/opensearch/sql/legacy/unittest/parser/SqlParserTest.java

legacy/src/main/java/org/opensearch/sql/legacy/utils/Util.java

legacy/src/main/java/org/opensearch/sql/legacy/antlr/visitor/AntlrSqlParseTreeVisitor.java

dai-chen

Did you remove your test accidentally? I don't see any UT or IT now.

RyanL1997 · 2025-09-30T23:09:27Z

Added the new tests in AntlrSqlParseTreeVisitorTest.java cc @dai-chen

dai-chen · 2025-09-30T23:11:09Z

legacy/src/main/java/org/opensearch/sql/legacy/antlr/visitor/AntlrSqlParseTreeVisitor.java

+            if (ctx.tableSource().size() > 1) {
+              return true;
+            }
+
+            for (int i = 0; i < ctx.tableSource().size(); i++) {
+              Boolean hasJoinInTableSource = visit(ctx.tableSource(i));
+              if (hasJoinInTableSource != null && hasJoinInTableSource) {
+                return true;
+              }
+            }
+            return false;


what's this trying to check?

transferring some offline communication with @dai-chen here: we should limit the scope of this for only check the explicit join only - so removed this section.

Signed-off-by: Jialiang Liang <jiallian@amazon.com>

RyanL1997 · 2025-10-01T01:54:33Z

#4378 is failing at the same

Swiddis · 2025-10-01T02:05:06Z

Failures exist in main and are only with recently added tests, think this is ok to merge

* main-apple: (218 commits) Add ignorePrometheus Flag for integTest and docTest (opensearch-project#4442) Create fab-radar.yml PPL `fillnull` command enhancement (opensearch-project#4421) reverting to _doc + _id (opensearch-project#4435) Support `multisearch` command in calcite (opensearch-project#4332) Add 3.3 release notes (opensearch-project#4422) (opensearch-project#4423) [SQL/PPL] Fix the `count(*)` and `dc(field)` to be capped at MAX_INTEGER opensearch-project#4416 (opensearch-project#4418) Change the default search sort tiebreaker to `_shard_doc` for PIT search (opensearch-project#4378) [Enhancement] Add error handling for known limitation of sql `JOIN` (opensearch-project#4344) Bugfix: SQL type mapping for legacy JDBC output (opensearch-project#3613) Version bump: 3.3 (opensearch-project#4417) Add max/min eval functions (opensearch-project#4333) Support time modifiers in search command (opensearch-project#4224) Fix numbered token bug and make it optional output in patterns command (opensearch-project#4402) refactor span (opensearch-project#4334) Move release notes categories (opensearch-project#3818) [Doc] Enable doctest with Calcite (opensearch-project#4379) Mod function should return decimal instead of float when handle the operands are decimal literal (opensearch-project#4407) Scale of decimal literal should always be positive in Calcite (opensearch-project#4401) Enable Calcite by default and implicit fallback the unsupported commands (opensearch-project#4372) ...

RyanL1997 added enhancement New feature or request SQL labels Sep 22, 2025

RyanL1997 changed the title ~~[Enhancement] Better error handling for known limitation of sql JOIN~~ [Enhancement] Add error handling for known limitation of sql JOIN Sep 22, 2025

RyanL1997 marked this pull request as ready for review September 22, 2025 15:23

Swiddis reviewed Sep 23, 2025

View reviewed changes

Swiddis previously approved these changes Sep 25, 2025

View reviewed changes

RyanL1997 dismissed Swiddis’s stale review via e9165de September 25, 2025 16:26

Swiddis previously approved these changes Sep 25, 2025

View reviewed changes

dai-chen reviewed Sep 25, 2025

View reviewed changes

legacy/src/main/java/org/opensearch/sql/legacy/utils/Util.java Outdated Show resolved Hide resolved

RyanL1997 dismissed Swiddis’s stale review via fcbf540 September 26, 2025 02:12

Swiddis added the v3.3.0 label Sep 29, 2025

dai-chen reviewed Sep 29, 2025

View reviewed changes

legacy/src/main/java/org/opensearch/sql/legacy/antlr/visitor/AntlrSqlParseTreeVisitor.java Outdated Show resolved Hide resolved

dai-chen reviewed Sep 30, 2025

View reviewed changes

RyanL1997 added 15 commits September 30, 2025 16:30

[Enhancement] Better error handling for known limitation of sql JOIN

427f11b

Signed-off-by: Jialiang Liang <jiallian@amazon.com>

spotless fix

c4f7d6f

Signed-off-by: Jialiang Liang <jiallian@amazon.com>

add tests

130c775

Signed-off-by: Jialiang Liang <jiallian@amazon.com>

simeon - dynamic versioning for doc link

71f07ac

Signed-off-by: Jialiang Liang <jiallian@amazon.com>

spotless fix

a6e4765

Signed-off-by: Jialiang Liang <jiallian@amazon.com>

simeon - extract error msg into utils

18c2a59

Signed-off-by: Jialiang Liang <jiallian@amazon.com>

simeon - table driven tests

0593ddd

Signed-off-by: Jialiang Liang <jiallian@amazon.com>

simeon - join select constructor

cc2808e

Signed-off-by: Jialiang Liang <jiallian@amazon.com>

chen - antlr check instead

237297d

Signed-off-by: Jialiang Liang <jiallian@amazon.com>

chen - fix test and explictly allow nested field

79cc77c

Signed-off-by: Jialiang Liang <jiallian@amazon.com>

remove some comments

bb39f25

Signed-off-by: Jialiang Liang <jiallian@amazon.com>

chen - simplify the detection logic of join

c335106

Signed-off-by: Jialiang Liang <jiallian@amazon.com>

clean up debugging

1829fad

Signed-off-by: Jialiang Liang <jiallian@amazon.com>

chen - add back testing

47e9bd2

Signed-off-by: Jialiang Liang <jiallian@amazon.com>

chen remove the check for implicit join

1634610

Signed-off-by: Jialiang Liang <jiallian@amazon.com>

RyanL1997 force-pushed the join-limitation-error-handling branch from 9240c00 to 1634610 Compare September 30, 2025 23:30

dai-chen approved these changes Oct 1, 2025

View reviewed changes

Swiddis approved these changes Oct 1, 2025

View reviewed changes

RyanL1997 mentioned this pull request Oct 1, 2025

Change the default search sort tiebreaker to _shard_doc for PIT search #4378

Merged

8 tasks

Swiddis merged commit 649ce41 into opensearch-project:main Oct 1, 2025
48 of 52 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Enhancement] Add error handling for known limitation of sql `JOIN`#4344

[Enhancement] Add error handling for known limitation of sql `JOIN`#4344
Swiddis merged 15 commits intoopensearch-project:mainfrom
RyanL1997:join-limitation-error-handling

RyanL1997 commented Sep 22, 2025 •

edited

Loading

Uh oh!

Swiddis left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dai-chen left a comment •

edited

Loading

Uh oh!

RyanL1997 commented Sep 30, 2025 •

edited

Loading

Uh oh!

dai-chen Sep 30, 2025

Uh oh!

RyanL1997 Sep 30, 2025

Uh oh!

RyanL1997 commented Oct 1, 2025

Uh oh!

Swiddis commented Oct 1, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

RyanL1997 commented Sep 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Check List

Uh oh!

Swiddis left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dai-chen left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RyanL1997 commented Sep 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dai-chen Sep 30, 2025

Choose a reason for hiding this comment

Uh oh!

RyanL1997 Sep 30, 2025

Choose a reason for hiding this comment

Uh oh!

RyanL1997 commented Oct 1, 2025

Uh oh!

Swiddis commented Oct 1, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

RyanL1997 commented Sep 22, 2025 •

edited

Loading

dai-chen left a comment •

edited

Loading

RyanL1997 commented Sep 30, 2025 •

edited

Loading