[BugFix] Fix empty SQL query failing with internal NullPointerException (#5879)

[RyanL1997]

Description

An empty SQL query ({"query": ""}), or one that is only whitespace, only a comment, or only ;, returned a 400 whose body exposed an internal NullPointerException (... because "statement" is null). This happened on _plugins/_sql, _plugins/_sql/_explain and ?format=jdbc. Calcite is not involved: the v2 SQL path does not use it (Calcite applies to PPL only, see QueryService.shouldUseCalcite).

Root cause: the SQL grammar's root rule sqlStatement? SEMI? EOF accepts input with no statement, so parsing succeeds and AstStatementBuilder returns a null Statement, which QueryPlanFactory.create then dereferences. This is the SQL counterpart of the PPL bug #5867.

Fix: AstStatementBuilder.visitRoot throws IllegalArgumentException("Query is empty") when the root has no statement. The grammar is unchanged.

Resulting error: IllegalArgumentException is a client error in RestSqlAction, so the request returns 400. It is not a SyntaxCheckException, so RestSQLQueryAction does not fall back to the legacy engine. Other syntax errors (e.g. SELEC 1) still fall back as before.

{"error": {"reason": "Invalid SQL query", "details": "Query is empty", "type": "IllegalArgumentException"}, "status": 400}

The unified-API SqlV2QueryParser uses the same AstStatementBuilder, so it no longer throws NullPointerException: Cannot invoke "Object.getClass()" because "statement" is null on the same input. It now throws the same IllegalArgumentException.

Related Issues

Resolves #5879
PPL counterpart: #5867, fixed by #5868

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.

[RyanL1997]

Decision Log

Root Cause: root : sqlStatement? SEMI? EOF accepts input with no statement: empty, whitespace, comment-only, or a bare ;. AstStatementBuilder then returns null, and QueryPlanFactory.create throws an NPE, which the response body exposes.

Approach (adopted from review): AstStatementBuilder.visitRoot throws IllegalArgumentException("Query is empty") when the root has no statement. RestSqlAction maps it to 400, and since it is not a SyntaxCheckException it skips the legacy fallback. The api module's SqlV2QueryParser uses the same builder, so its NPE is fixed too. The grammar is unchanged.

Alternatives Rejected:

• Grammar change plus a hasNoStatement fallback skip (previous revision): it returned the same type as PPL, but needed a lexer-based check and a change to the legacy fallback listener.
• Grammar change alone: the resulting SyntaxCheckException falls back to legacy, which replaces it with a misleading error ("Query must start with SELECT...").
• A SyntaxCheckException subclass in common: it would change PPL's response type.
• Null-checks in SQLService / QueryPlanFactory: these treat the symptom and miss SqlV2QueryParser.

Result: on _plugins/_sql, _explain and ?format=jdbc, all statement-less inputs return 400 IllegalArgumentException "Query is empty", with no fallback in the server log. SELEC 1 still falls back. On main they returned the leaked NPE. Calcite is not involved, since it applies to PPL only.

Things to Watch:

• SQL returns IllegalArgumentException for an empty query, while PPL ([BugFix] Fix empty PPL query returning 500 NullPointerException instead of 400 (#5867) #5868) returns SyntaxCheckException. Both are 400.
• Input that the lexer drops entirely (/*! ... */, or an unrecognized character such as #) also has no statement, and returns "Query is empty".
• The PR adds YAML API specs sql.json and sql.explain.json, because none existed.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 4ff7fa8)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ No major issues detected

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 4ff7fa8

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Add null check for context The null check should be performed before calling super.visitRoot(ctx) to ensure proper error handling. However, consider also checking if the context itself is null to prevent potential NullPointerException if ctx is null before accessing ctx.sqlStatement(). sql/src/main/java/org/opensearch/sql/sql/parser/AstStatementBuilder.java [26-32] @Override public Statement visitRoot(OpenSearchSQLParser.RootContext ctx) { - if (ctx.sqlStatement() == null) { + if (ctx == null || ctx.sqlStatement() == null) { throw new IllegalArgumentException("Query is empty"); } return super.visitRoot(ctx); } Suggestion importance[1-10]: 3 __ Why: While adding a null check for ctx could prevent a potential NullPointerException, this is likely unnecessary as the visitor pattern typically guarantees non-null contexts. The suggestion addresses a theoretical edge case but may not reflect actual usage patterns in the codebase.	Low

---

Previous suggestions

Suggestions up to commit bc88f54

Category	Suggestion	Impact
General	Optimize token stream processing efficiency The method calls getAllTokens() which consumes the entire token stream. This could be inefficient for large queries. Consider using anyMatch with negation or short-circuiting to avoid processing all tokens when an early mismatch is found. sql/src/main/java/org/opensearch/sql/sql/antlr/SQLSyntaxParser.java [50-56] public static boolean hasNoStatement(String query) { return createLexer(query).getAllTokens().stream() - .allMatch( + .noneMatch( token -> - token.getChannel() != Token.DEFAULT_CHANNEL - || token.getType() == OpenSearchSQLLexer.SEMI); + token.getChannel() == Token.DEFAULT_CHANNEL + && token.getType() != OpenSearchSQLLexer.SEMI); } Suggestion importance[1-10]: 5 __ Why: Using noneMatch instead of allMatch is semantically equivalent and slightly more readable. However, both approaches consume all tokens via getAllTokens(), so there's no actual performance improvement. The suggestion doesn't address the underlying efficiency concern it claims to solve.	Low
	Clarify exception handling logic flow The condition logic may not handle all edge cases correctly. If hasNoStatement(request) returns true for a SyntaxCheckException, the exception bypasses the fallback handler and goes to next.onFailure(e). Consider whether this is the intended behavior for all scenarios, especially when the next handler might not be equipped to handle syntax errors. legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSQLQueryAction.java [141-146] -if ((e instanceof SyntaxCheckException && !hasNoStatement(request)) - || e instanceof UnsupportedCursorRequestException) { +if (e instanceof UnsupportedCursorRequestException) { + fallBackHandler.accept(channel, e); +} else if (e instanceof SyntaxCheckException && hasNoStatement(request)) { + next.onFailure(e); +} else if (e instanceof SyntaxCheckException) { fallBackHandler.accept(channel, e); } else { next.onFailure(e); } Suggestion importance[1-10]: 3 __ Why: The suggested refactoring makes the logic more explicit but doesn't change the behavior. The original code correctly handles the case where SyntaxCheckException with no statement should not fall back. The improved readability is marginal.	Low

Suggestions up to commit 403a3ad

Category	Suggestion	Impact
General	Use JSON library for safe escaping The test queries containing newlines or special characters may not be properly escaped in the JSON string format. The query "-- line" could cause issues if it contains actual newlines. Use proper JSON escaping or a JSON library to construct the request body safely. integ-test/src/test/java/org/opensearch/sql/sql/QueryValidationIT.java [124-133] for (String query : new String[] {"", " ", "/* only a comment */", "-- line", ";"}) { for (boolean explain : new boolean[] {false, true}) { - Request request = - getSqlRequest(String.format(Locale.ROOT, "{\"query\": \"%s\"}", query), explain); + String jsonBody = new JSONObject().put("query", query).toString(); + Request request = getSqlRequest(jsonBody, explain); ResponseException e = assertThrows(ResponseException.class, () -> client().performRequest(request)); String context = "explain=" + explain + ", query=[" + query + "]"; assertEquals(context, 400, e.getResponse().getStatusLine().getStatusCode()); assertFalse(context, e.getMessage().contains("NullPointerException")); } } Suggestion importance[1-10]: 3 __ Why: While using a JSON library could improve robustness, the current String.format approach with simple test strings is adequate for this test case. The test queries don't contain actual newlines that would break JSON formatting, making this a minor improvement rather than a critical fix.	Low

Suggestions up to commit 62586d3

Category	Suggestion	Impact
General	Ensure state cleanup per iteration The test doesn't restore the Calcite state between iterations. If enableCalcite() fails or throws an exception during the first iteration, subsequent iterations may run with an unexpected state. Consider wrapping each iteration in a try-finally block to ensure proper cleanup. integ-test/src/test/java/org/opensearch/sql/sql/QueryValidationIT.java [127-145] for (boolean calcite : new boolean[] {false, true}) { - if (calcite) { - enableCalcite(); - } else { + try { + if (calcite) { + enableCalcite(); + } else { + disableCalcite(); + } + for (String query : new String[] {"", " ", "/* only a comment */", "-- line", ";"}) { + for (boolean explain : new boolean[] {false, true}) { + Request request = + getSqlRequest(String.format(Locale.ROOT, "{\"query\": \"%s\"}", query), explain); + ResponseException e = + assertThrows(ResponseException.class, () -> client().performRequest(request)); + String context = + "calcite=" + calcite + ", explain=" + explain + ", query=[" + query + "]"; + assertEquals(context, 400, e.getResponse().getStatusLine().getStatusCode()); + assertFalse(context, e.getMessage().contains("NullPointerException")); + } + } + } finally { disableCalcite(); - } - for (String query : new String[] {"", " ", "/* only a comment */", "-- line", ";"}) { - for (boolean explain : new boolean[] {false, true}) { - Request request = - getSqlRequest(String.format(Locale.ROOT, "{\"query\": \"%s\"}", query), explain); - ResponseException e = - assertThrows(ResponseException.class, () -> client().performRequest(request)); - String context = - "calcite=" + calcite + ", explain=" + explain + ", query=[" + query + "]"; - assertEquals(context, 400, e.getResponse().getStatusLine().getStatusCode()); - assertFalse(context, e.getMessage().contains("NullPointerException")); - } } } Suggestion importance[1-10]: 4 __ Why: The suggestion improves robustness by ensuring disableCalcite() is called after each iteration. However, the outer finally block at line 146-148 already handles cleanup, making this a minor improvement for intermediate failure scenarios.	Low

[github-actions]

Persistent review updated to latest commit 403a3ad

[github-actions]

Persistent review updated to latest commit bc88f54

[dai-chen]

To simplify this, probably we can keep the question mark and throw a different exception (e.g., IllegalArgumentException) in visitRoot when sqlStatement is null to avoid fallback?

[RyanL1997]

Fixed. The grammar is reverted, and visitRoot now throws IllegalArgumentException("Query is empty"), which returns 400 without falling back to legacy.

Previously I was trying to align with the PPL exception type on it, but in SQL any SyntaxCheckException falls back to legacy, so matching PPL needed an extra empty-query check in the fallback path. Your suggested approach avoids that with a much smaller change.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 63.88%. Comparing base (4d98393) to head (4ff7fa8).

❌ Your project check has failed because the head coverage (63.88%) is below the target coverage (99.00%). You can increase the head coverage or adjust the target coverage.

Additional details and impacted files

@@            Coverage Diff            @@
##               main    #5880   +/-   ##
=========================================
  Coverage     63.87%   63.88%           
- Complexity     9091     9093    +2     
=========================================
  Files           962      962           
  Lines         41033    41036    +3     
  Branches       4668     4668           
=========================================
+ Hits          26211    26214    +3     
  Misses        13944    13944           
  Partials        878      878           

Flag	Coverage Δ
sql-engine	63.88% <100.00%> (+<0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[github-actions]

Persistent review updated to latest commit 4ff7fa8