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 54a429e4cfb..767b8d90f1f 100644 --- a/api/src/main/java/org/opensearch/sql/api/UnifiedQueryPlanner.java +++ b/api/src/main/java/org/opensearch/sql/api/UnifiedQueryPlanner.java @@ -23,6 +23,7 @@ import org.opensearch.sql.ast.tree.UnresolvedPlan; import org.opensearch.sql.calcite.CalciteRelNodeVisitor; import org.opensearch.sql.common.antlr.SyntaxCheckException; +import org.opensearch.sql.common.error.ErrorReport; import org.opensearch.sql.executor.QueryType; /** @@ -71,6 +72,9 @@ public RelNode plan(String query) { }); } catch (SyntaxCheckException | UnsupportedOperationException e) { throw e; + } catch (ErrorReport e) { + if (e.getCause() instanceof SyntaxCheckException) throw e; + throw new IllegalStateException("Failed to plan query", e); } catch (Exception e) { throw new IllegalStateException("Failed to plan query", e); } 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 41ed12670f8..7b4bcb6ea71 100644 --- a/api/src/test/java/org/opensearch/sql/api/UnifiedQueryPlannerTest.java +++ b/api/src/test/java/org/opensearch/sql/api/UnifiedQueryPlannerTest.java @@ -13,7 +13,7 @@ import org.apache.calcite.schema.Schema; import org.apache.calcite.schema.impl.AbstractSchema; import org.junit.Test; -import org.opensearch.sql.common.antlr.SyntaxCheckException; +import org.opensearch.sql.common.error.ErrorReport; import org.opensearch.sql.executor.QueryType; public class UnifiedQueryPlannerTest extends UnifiedQueryTestBase { @@ -111,8 +111,11 @@ public void testUnsupportedStatementType() { planner.plan("explain source = catalog.employees"); // explain statement } - @Test(expected = SyntaxCheckException.class) + @Test public void testPlanPropagatingSyntaxCheckException() { - planner.plan("source = catalog.employees | eval"); // Trigger syntax error from parser + assertThrows( + ErrorReport.class, + () -> + planner.plan("source = catalog.employees | eval")); // Trigger syntax error from parser } } diff --git a/api/src/test/java/org/opensearch/sql/api/parser/UnifiedQueryParserTest.java b/api/src/test/java/org/opensearch/sql/api/parser/UnifiedQueryParserTest.java index 1b6b5181aef..d432876925e 100644 --- a/api/src/test/java/org/opensearch/sql/api/parser/UnifiedQueryParserTest.java +++ b/api/src/test/java/org/opensearch/sql/api/parser/UnifiedQueryParserTest.java @@ -28,7 +28,7 @@ import org.junit.Test; import org.opensearch.sql.api.UnifiedQueryTestBase; import org.opensearch.sql.ast.tree.UnresolvedPlan; -import org.opensearch.sql.common.antlr.SyntaxCheckException; +import org.opensearch.sql.common.error.ErrorReport; public class UnifiedQueryParserTest extends UnifiedQueryTestBase { @@ -77,7 +77,7 @@ public void testParseStats() { @Test public void testSyntaxErrorThrows() { - assertThrows(SyntaxCheckException.class, () -> context.getParser().parse("not a valid query")); + assertThrows(ErrorReport.class, () -> context.getParser().parse("not a valid query")); } private void assertEqual(String query, UnresolvedPlan expected) { diff --git a/async-query-core/src/main/java/org/opensearch/sql/spark/utils/SQLQueryUtils.java b/async-query-core/src/main/java/org/opensearch/sql/spark/utils/SQLQueryUtils.java index 1e21354d1eb..e4d9b9ee94d 100644 --- a/async-query-core/src/main/java/org/opensearch/sql/spark/utils/SQLQueryUtils.java +++ b/async-query-core/src/main/java/org/opensearch/sql/spark/utils/SQLQueryUtils.java @@ -17,7 +17,7 @@ import org.apache.logging.log4j.Logger; import org.opensearch.sql.common.antlr.CaseInsensitiveCharStream; import org.opensearch.sql.common.antlr.SyntaxAnalysisErrorListener; -import org.opensearch.sql.common.antlr.SyntaxCheckException; +import org.opensearch.sql.common.error.ErrorReport; import org.opensearch.sql.spark.antlr.parser.FlintSparkSqlExtensionsBaseVisitor; import org.opensearch.sql.spark.antlr.parser.FlintSparkSqlExtensionsLexer; import org.opensearch.sql.spark.antlr.parser.FlintSparkSqlExtensionsParser; @@ -85,7 +85,7 @@ public static boolean isFlintExtensionQuery(String sqlQuery) { try { flintSparkSqlExtensionsParser.statement(); return true; - } catch (SyntaxCheckException syntaxCheckException) { + } catch (ErrorReport e) { return false; } } diff --git a/common/src/main/java/org/opensearch/sql/common/antlr/SyntaxAnalysisErrorListener.java b/common/src/main/java/org/opensearch/sql/common/antlr/SyntaxAnalysisErrorListener.java index 9cf118f64ae..9bebc11833c 100644 --- a/common/src/main/java/org/opensearch/sql/common/antlr/SyntaxAnalysisErrorListener.java +++ b/common/src/main/java/org/opensearch/sql/common/antlr/SyntaxAnalysisErrorListener.java @@ -6,8 +6,11 @@ package org.opensearch.sql.common.antlr; import java.util.ArrayList; +import java.util.HashMap; import java.util.List; import java.util.Locale; +import java.util.Map; +import java.util.Optional; import org.antlr.v4.runtime.BaseErrorListener; import org.antlr.v4.runtime.CommonTokenStream; import org.antlr.v4.runtime.RecognitionException; @@ -15,6 +18,10 @@ import org.antlr.v4.runtime.Token; import org.antlr.v4.runtime.Vocabulary; import org.antlr.v4.runtime.misc.IntervalSet; +import org.opensearch.sql.common.antlr.suggestion.SyntaxErrorContext; +import org.opensearch.sql.common.antlr.suggestion.SyntaxErrorSuggestionRegistry; +import org.opensearch.sql.common.error.ErrorCode; +import org.opensearch.sql.common.error.ErrorReport; /** * Syntax analysis error listener that handles any syntax error by throwing exception with useful @@ -39,13 +46,42 @@ public void syntaxError( Token offendingToken = (Token) offendingSymbol; String query = tokens.getText(); - throw new SyntaxCheckException( + // Build the original error message for backward compatibility + String details = String.format( Locale.ROOT, "[%s] is not a valid term at this part of the query: '%s' <-- HERE. %s", getOffendingText(offendingToken), truncateQueryAtOffendingToken(query, offendingToken), - getDetails(recognizer, msg, e))); + getDetails(recognizer, msg, e)); + + // Create a SyntaxCheckException as the underlying cause + SyntaxCheckException cause = new SyntaxCheckException(details); + + // Build position information + Map position = new HashMap<>(); + position.put("line", line); + position.put("column", charPositionInLine); + + // Build ErrorReport with structured context + ErrorReport.Builder reportBuilder = + ErrorReport.wrap(cause) + .code(ErrorCode.SYNTAX_ERROR) + .location("while parsing the query") + .context("query", query) + .context("position", position) + .context("offending_token", getOffendingText(offendingToken)); + + // Use the suggestion registry to find pattern-based suggestions + SyntaxErrorContext context = + new SyntaxErrorContext(recognizer, offendingToken, tokens, query, e); + Optional customSuggestion = SyntaxErrorSuggestionRegistry.findSuggestion(context); + + if (customSuggestion.isPresent()) { + reportBuilder.suggestion(customSuggestion.get()); + } + + throw reportBuilder.build(); } private String getOffendingText(Token offendingToken) { diff --git a/common/src/main/java/org/opensearch/sql/common/antlr/suggestion/ExpectedTokensSuggestionProvider.java b/common/src/main/java/org/opensearch/sql/common/antlr/suggestion/ExpectedTokensSuggestionProvider.java new file mode 100644 index 00000000000..d2b16a10fc5 --- /dev/null +++ b/common/src/main/java/org/opensearch/sql/common/antlr/suggestion/ExpectedTokensSuggestionProvider.java @@ -0,0 +1,45 @@ +/* + * Copyright OpenSearch Contributors + * SPDX-License-Identifier: Apache-2.0 + */ + +package org.opensearch.sql.common.antlr.suggestion; + +import java.util.ArrayList; +import java.util.List; +import java.util.Optional; +import org.antlr.v4.runtime.RecognitionException; +import org.antlr.v4.runtime.Vocabulary; +import org.antlr.v4.runtime.misc.IntervalSet; + +/** Fallback provider: surface the parser's expected-tokens set. */ +public class ExpectedTokensSuggestionProvider implements SyntaxErrorSuggestionProvider { + private static final int MAX = 5; + + @Override + public Optional getSuggestion(SyntaxErrorContext ctx) { + RecognitionException e = ctx.getException(); + if (e == null) return Optional.empty(); + IntervalSet expected = e.getExpectedTokens(); + if (expected == null || expected.size() == 0) return Optional.empty(); + List types = expected.toList(); + if (types.isEmpty()) return Optional.empty(); + Vocabulary vocab = ctx.getRecognizer().getVocabulary(); + List names = new ArrayList<>(MAX); + for (int type : types.subList(0, Math.min(types.size(), MAX))) { + names.add(vocab.getDisplayName(type)); + } + String msg = + types.size() > MAX + ? String.format( + "Expected one of %d possible tokens. Examples: %s", + types.size(), String.join(", ", names)) + : "Expected tokens: " + String.join(", ", names); + return Optional.of(msg); + } + + @Override + public int getPriority() { + return Integer.MAX_VALUE; + } +} diff --git a/common/src/main/java/org/opensearch/sql/common/antlr/suggestion/SelectStarSuggestionProvider.java b/common/src/main/java/org/opensearch/sql/common/antlr/suggestion/SelectStarSuggestionProvider.java new file mode 100644 index 00000000000..c925bde79f8 --- /dev/null +++ b/common/src/main/java/org/opensearch/sql/common/antlr/suggestion/SelectStarSuggestionProvider.java @@ -0,0 +1,73 @@ +/* + * Copyright OpenSearch Contributors + * SPDX-License-Identifier: Apache-2.0 + */ + +package org.opensearch.sql.common.antlr.suggestion; + +import java.util.Optional; +import java.util.regex.Pattern; + +/** Detects SELECT * FROM pattern in PPL context. */ +public class SelectStarSuggestionProvider implements SyntaxErrorSuggestionProvider { + private static final Pattern SELECT_STAR_PATTERN = Pattern.compile("(?i)^\\s*select\\s+\\*.*"); + private static final Pattern SELECT_FIELDS_PATTERN = + Pattern.compile("(?i)^\\s*select\\s+[a-zA-Z_][a-zA-Z0-9_]*.*"); + // Pattern to extract table name from "SELECT ... FROM table_name" queries + private static final Pattern TABLE_NAME_PATTERN = + Pattern.compile("(?i).*\\bfrom\\s+([a-zA-Z_][a-zA-Z0-9_\\.]*)", Pattern.DOTALL); + + @Override + public Optional getSuggestion(SyntaxErrorContext context) { + // Only suggest PPL syntax when the error originated from the PPL parser. + if (context.getRecognizer() == null + || !context.getRecognizer().getGrammarFileName().contains("PPL")) { + return Optional.empty(); + } + + // Check if query starts with "select" (case-insensitive) + String query = context.getQuery().trim(); + if (!query.toLowerCase().startsWith("select")) { + return Optional.empty(); + } + + // Extract table name if available + String tableName = extractTableName(query); + String tableRef = tableName != null ? tableName : "index"; + + // Check if this looks like SQL SELECT * syntax + if (SELECT_STAR_PATTERN.matcher(query).matches()) { + return Optional.of( + "PPL uses 'source=" + + tableRef + + " | fields *' instead of 'SELECT * FROM " + + tableRef + + "'"); + } + + // Check if this looks like SQL SELECT fields syntax + if (SELECT_FIELDS_PATTERN.matcher(query).matches()) { + return Optional.of( + "PPL uses 'source=" + + tableRef + + " | fields field1, field2' instead of 'SELECT field1, field2 FROM " + + tableRef + + "'"); + } + + return Optional.empty(); + } + + private String extractTableName(String query) { + java.util.regex.Matcher matcher = TABLE_NAME_PATTERN.matcher(query); + if (matcher.find()) { + return matcher.group(1); + } + return null; + } + + @Override + public int getPriority() { + return 10; // High priority for SQL/PPL syntax confusion + } +} diff --git a/common/src/main/java/org/opensearch/sql/common/antlr/suggestion/SyntaxErrorContext.java b/common/src/main/java/org/opensearch/sql/common/antlr/suggestion/SyntaxErrorContext.java new file mode 100644 index 00000000000..e0a891f8aee --- /dev/null +++ b/common/src/main/java/org/opensearch/sql/common/antlr/suggestion/SyntaxErrorContext.java @@ -0,0 +1,40 @@ +/* + * Copyright OpenSearch Contributors + * SPDX-License-Identifier: Apache-2.0 + */ + +package org.opensearch.sql.common.antlr.suggestion; + +import java.util.List; +import lombok.Getter; +import lombok.RequiredArgsConstructor; +import org.antlr.v4.runtime.CommonTokenStream; +import org.antlr.v4.runtime.RecognitionException; +import org.antlr.v4.runtime.Recognizer; +import org.antlr.v4.runtime.Token; + +/** Context passed to each {@link SyntaxErrorSuggestionProvider} for pattern matching. */ +@RequiredArgsConstructor +@Getter +public class SyntaxErrorContext { + private final Recognizer recognizer; + private final Token offendingToken; + private final CommonTokenStream tokens; + private final String query; + private final RecognitionException exception; + + public String getOffendingText() { + return offendingToken == null ? "" : offendingToken.getText(); + } + + /** Text of the query after the offending token (trimmed). */ + public String getRemainingQuery() { + if (offendingToken == null) return ""; + int end = offendingToken.getStopIndex() + 1; + return end >= query.length() ? "" : query.substring(end); + } + + public List getAllTokens() { + return tokens.getTokens(); + } +} diff --git a/common/src/main/java/org/opensearch/sql/common/antlr/suggestion/SyntaxErrorSuggestionProvider.java b/common/src/main/java/org/opensearch/sql/common/antlr/suggestion/SyntaxErrorSuggestionProvider.java new file mode 100644 index 00000000000..76efb323e92 --- /dev/null +++ b/common/src/main/java/org/opensearch/sql/common/antlr/suggestion/SyntaxErrorSuggestionProvider.java @@ -0,0 +1,23 @@ +/* + * Copyright OpenSearch Contributors + * SPDX-License-Identifier: Apache-2.0 + */ + +package org.opensearch.sql.common.antlr.suggestion; + +import java.util.Optional; + +/** + * Provides syntax error suggestions for a specific pattern. Implementations must be stateless and + * thread-safe. + */ +public interface SyntaxErrorSuggestionProvider { + /** Return a suggestion for the given error context, or empty if no suggestion applies. */ + Optional getSuggestion(SyntaxErrorContext context); + + /** + * Priority for this provider (lower = higher priority). Providers with lower priority values are + * checked first. + */ + int getPriority(); +} diff --git a/common/src/main/java/org/opensearch/sql/common/antlr/suggestion/SyntaxErrorSuggestionRegistry.java b/common/src/main/java/org/opensearch/sql/common/antlr/suggestion/SyntaxErrorSuggestionRegistry.java new file mode 100644 index 00000000000..64db04311f0 --- /dev/null +++ b/common/src/main/java/org/opensearch/sql/common/antlr/suggestion/SyntaxErrorSuggestionRegistry.java @@ -0,0 +1,43 @@ +/* + * Copyright OpenSearch Contributors + * SPDX-License-Identifier: Apache-2.0 + */ + +package org.opensearch.sql.common.antlr.suggestion; + +import java.util.Arrays; +import java.util.Comparator; +import java.util.Optional; +import java.util.concurrent.CopyOnWriteArrayList; + +/** Registry of syntax-error suggestion providers, evaluated in priority order. */ +public final class SyntaxErrorSuggestionRegistry { + // CopyOnWriteArrayList: safe iteration during concurrent register() calls. + private static final CopyOnWriteArrayList PROVIDERS = + new CopyOnWriteArrayList<>(); + + static { + register( + new SelectStarSuggestionProvider(), + new UnmatchedParenthesesSuggestionProvider(), + new ExpectedTokensSuggestionProvider()); + } + + private SyntaxErrorSuggestionRegistry() {} + + public static void register(SyntaxErrorSuggestionProvider... providers) { + PROVIDERS.addAll(Arrays.asList(providers)); + PROVIDERS.sort(Comparator.comparingInt(SyntaxErrorSuggestionProvider::getPriority)); + } + + /** Returns suggestion from the first matching provider (by priority); empty otherwise. */ + public static Optional findSuggestion(SyntaxErrorContext context) { + for (SyntaxErrorSuggestionProvider provider : PROVIDERS) { + Optional suggestion = provider.getSuggestion(context); + if (suggestion.isPresent()) { + return suggestion; + } + } + return Optional.empty(); + } +} diff --git a/common/src/main/java/org/opensearch/sql/common/antlr/suggestion/UnmatchedParenthesesSuggestionProvider.java b/common/src/main/java/org/opensearch/sql/common/antlr/suggestion/UnmatchedParenthesesSuggestionProvider.java new file mode 100644 index 00000000000..121871d5be8 --- /dev/null +++ b/common/src/main/java/org/opensearch/sql/common/antlr/suggestion/UnmatchedParenthesesSuggestionProvider.java @@ -0,0 +1,66 @@ +/* + * Copyright OpenSearch Contributors + * SPDX-License-Identifier: Apache-2.0 + */ + +package org.opensearch.sql.common.antlr.suggestion; + +import java.util.Optional; +import org.antlr.v4.runtime.Token; + +/** Detects unmatched parentheses in queries and suggests balancing them. */ +public class UnmatchedParenthesesSuggestionProvider implements SyntaxErrorSuggestionProvider { + @Override + public Optional getSuggestion(SyntaxErrorContext ctx) { + if (ctx.getQuery() == null || ctx.getQuery().isEmpty()) return Optional.empty(); + + // Count paren tokens on the default channel, which skips string literals, comments, + // and whitespace so we don't get false positives from e.g. `where msg = "ignore )"`. + int openParens = 0; + int closeParens = 0; + for (Token t : ctx.getAllTokens()) { + if (t.getChannel() != Token.DEFAULT_CHANNEL) continue; + String text = t.getText(); + if ("(".equals(text)) { + openParens++; + } else if (")".equals(text)) { + closeParens++; + } + } + + // If we have more opening than closing parentheses + if (openParens > closeParens) { + int missing = openParens - closeParens; + if (missing == 1) { + return Optional.of( + "Missing closing parenthesis ')'. Check that all parentheses are balanced"); + } else { + return Optional.of( + String.format( + "Missing %d closing parentheses ')'. Check that all parentheses are balanced", + missing)); + } + } + + // If we have more closing than opening parentheses + if (closeParens > openParens) { + int extra = closeParens - openParens; + if (extra == 1) { + return Optional.of( + "Extra closing parenthesis ')'. Check that all parentheses are balanced"); + } else { + return Optional.of( + String.format( + "Extra %d closing parentheses ')'. Check that all parentheses are balanced", + extra)); + } + } + + return Optional.empty(); + } + + @Override + public int getPriority() { + return 20; // High priority since unmatched parentheses are common + } +} diff --git a/legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSQLQueryAction.java b/legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSQLQueryAction.java index 21badf79412..998fa9576b1 100644 --- a/legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSQLQueryAction.java +++ b/legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSQLQueryAction.java @@ -20,6 +20,7 @@ import org.opensearch.rest.RestChannel; import org.opensearch.rest.RestRequest; import org.opensearch.sql.common.antlr.SyntaxCheckException; +import org.opensearch.sql.common.error.ErrorReport; import org.opensearch.sql.common.response.ResponseListener; import org.opensearch.sql.common.utils.QueryContext; import org.opensearch.sql.exception.UnsupportedCursorRequestException; @@ -131,7 +132,10 @@ public void onResponse(T response) { @Override public void onFailure(Exception e) { - if (e instanceof SyntaxCheckException || e instanceof UnsupportedCursorRequestException) { + if (e instanceof SyntaxCheckException + || e instanceof UnsupportedCursorRequestException + || (e instanceof ErrorReport + && ((ErrorReport) e).getCause() instanceof SyntaxCheckException)) { fallBackHandler.accept(channel, e); } else { next.onFailure(e); diff --git a/legacy/src/test/java/org/opensearch/sql/legacy/plugin/RestSQLQueryActionTest.java b/legacy/src/test/java/org/opensearch/sql/legacy/plugin/RestSQLQueryActionTest.java index 9ce1ac8f3a4..164ed1ae56d 100644 --- a/legacy/src/test/java/org/opensearch/sql/legacy/plugin/RestSQLQueryActionTest.java +++ b/legacy/src/test/java/org/opensearch/sql/legacy/plugin/RestSQLQueryActionTest.java @@ -28,6 +28,7 @@ import org.opensearch.rest.RestChannel; import org.opensearch.rest.RestRequest; import org.opensearch.sql.common.antlr.SyntaxCheckException; +import org.opensearch.sql.common.error.ErrorReport; import org.opensearch.sql.executor.QueryManager; import org.opensearch.sql.executor.execution.QueryPlanFactory; import org.opensearch.sql.sql.SQLService; @@ -127,7 +128,7 @@ public void queryThatNotSupportIsHandledByFallbackHandler() throws Exception { request, (channel, exception) -> { fallback.set(true); - assertTrue(exception instanceof SyntaxCheckException); + assertTrue(exception instanceof ErrorReport); }, (channel, exception) -> { fail(); diff --git a/ppl/src/test/java/org/opensearch/sql/ppl/antlr/PPLSyntaxParserTest.java b/ppl/src/test/java/org/opensearch/sql/ppl/antlr/PPLSyntaxParserTest.java index 1238f1e7e6c..015d23fd68a 100644 --- a/ppl/src/test/java/org/opensearch/sql/ppl/antlr/PPLSyntaxParserTest.java +++ b/ppl/src/test/java/org/opensearch/sql/ppl/antlr/PPLSyntaxParserTest.java @@ -15,7 +15,7 @@ import org.junit.Test; import org.junit.rules.ExpectedException; import org.opensearch.sql.common.antlr.CaseInsensitiveCharStream; -import org.opensearch.sql.common.antlr.SyntaxCheckException; +import org.opensearch.sql.common.error.ErrorReport; import org.opensearch.sql.ppl.antlr.parser.OpenSearchPPLLexer; import org.opensearch.sql.ppl.antlr.parser.OpenSearchPPLParser; @@ -667,7 +667,7 @@ public void testCanParseGetFormatFunction() { @Test public void testCannotParseGetFormatFunctionWithBadArg() { assertThrows( - SyntaxCheckException.class, + ErrorReport.class, () -> new PPLSyntaxParser() .parse("SOURCE=test | eval k = GET_FORMAT(NONSENSE_ARG,'INTERNAL')")); @@ -994,7 +994,51 @@ public void testSubSearchWithTrailingPipeShouldPass() { @Test public void testPipeWithInvalidCommandShouldFail() { assertThrows( - SyntaxCheckException.class, - () -> new PPLSyntaxParser().parse("source=t | | 123invalidcommand")); + ErrorReport.class, () -> new PPLSyntaxParser().parse("source=t | | 123invalidcommand")); + } + + /** Tests that SQL syntax in PPL context provides helpful conversion suggestions. */ + @Test + public void testSelectStarSyntaxProvidesPPLSuggestion() { + ErrorReport error = + assertThrows(ErrorReport.class, () -> new PPLSyntaxParser().parse("select * from test")); + + assertNotNull("Error should have a suggestion", error.getSuggestion()); + assertTrue( + "Suggestion should mention PPL source syntax", + error.getSuggestion().contains("PPL uses 'source=index | fields *'")); + assertTrue( + "Suggestion should mention SELECT FROM syntax", + error.getSuggestion().contains("SELECT * FROM index")); + } + + @Test + public void testSelectFieldsSyntaxProvidesPPLSuggestion() { + ErrorReport error = + assertThrows( + ErrorReport.class, () -> new PPLSyntaxParser().parse("select name, age from users")); + + assertNotNull("Error should have a suggestion", error.getSuggestion()); + assertTrue( + "Suggestion should mention PPL fields syntax", + error.getSuggestion().contains("PPL uses 'source=index | fields field1, field2'")); + assertTrue( + "Suggestion should mention SELECT fields syntax", + error.getSuggestion().contains("SELECT field1, field2 FROM index")); + } + + @Test + public void testUnmatchedParenthesesProvidesSuggestion() { + ErrorReport error = + assertThrows( + ErrorReport.class, () -> new PPLSyntaxParser().parse("source=test | eval x = (1 + 2")); + + assertNotNull("Error should have a suggestion", error.getSuggestion()); + assertTrue( + "Suggestion should mention missing closing parenthesis", + error.getSuggestion().contains("Missing closing parenthesis ')'")); + assertTrue( + "Suggestion should mention balanced parentheses", + error.getSuggestion().contains("Check that all parentheses are balanced")); } } diff --git a/ppl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLReplaceTest.java b/ppl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLReplaceTest.java index 5f6f2beb76d..6011c2f6562 100644 --- a/ppl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLReplaceTest.java +++ b/ppl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLReplaceTest.java @@ -8,7 +8,7 @@ import org.apache.calcite.rel.RelNode; import org.apache.calcite.test.CalciteAssert; import org.junit.Test; -import org.opensearch.sql.common.antlr.SyntaxCheckException; +import org.opensearch.sql.common.error.ErrorReport; public class CalcitePPLReplaceTest extends CalcitePPLAbstractTest { @@ -140,19 +140,19 @@ public void testReplaceWithPipeline() { verifyPPLToSparkSQL(root, expectedSparkSql); } - @Test(expected = SyntaxCheckException.class) + @Test(expected = ErrorReport.class) public void testReplaceWithoutWithKeywordShouldFail() { String ppl = "source=EMP | replace \"CLERK\" \"EMPLOYEE\" IN JOB"; getRelNode(ppl); } - @Test(expected = SyntaxCheckException.class) + @Test(expected = ErrorReport.class) public void testReplaceWithoutInKeywordShouldFail() { String ppl = "source=EMP | replace \"CLERK\" WITH \"EMPLOYEE\" JOB"; getRelNode(ppl); } - @Test(expected = SyntaxCheckException.class) + @Test(expected = ErrorReport.class) public void testReplaceWithExpressionShouldFail() { String ppl = "source=EMP | replace EMPNO + 1 WITH \"EMPLOYEE\" IN JOB"; getRelNode(ppl); @@ -170,13 +170,13 @@ public void testReplaceWithMultipleInKeywordsShouldFail() { getRelNode(ppl); } - @Test(expected = SyntaxCheckException.class) + @Test(expected = ErrorReport.class) public void testReplaceWithMissingQuotesShouldFail() { String ppl = "source=EMP | replace CLERK WITH EMPLOYEE IN JOB"; getRelNode(ppl); } - @Test(expected = SyntaxCheckException.class) + @Test(expected = ErrorReport.class) public void testReplaceWithMissingReplacementValueShouldFail() { String ppl = "source=EMP | replace \"CLERK\" WITH IN JOB"; getRelNode(ppl); @@ -311,7 +311,7 @@ public void testReplaceWithMultiplePairsSequentialApplication() { verifyPPLToSparkSQL(root, expectedSparkSql); } - @Test(expected = SyntaxCheckException.class) + @Test(expected = ErrorReport.class) public void testReplaceWithMultiplePairsMissingWithKeywordShouldFail() { // Missing WITH keyword between pairs String ppl = @@ -319,7 +319,7 @@ public void testReplaceWithMultiplePairsMissingWithKeywordShouldFail() { getRelNode(ppl); } - @Test(expected = SyntaxCheckException.class) + @Test(expected = ErrorReport.class) public void testReplaceWithMultiplePairsTrailingCommaShouldFail() { // Trailing comma after last pair String ppl = "source=EMP | replace \"CLERK\" WITH \"EMPLOYEE\", IN JOB"; diff --git a/ppl/src/test/java/org/opensearch/sql/ppl/parser/AstBuilderTest.java b/ppl/src/test/java/org/opensearch/sql/ppl/parser/AstBuilderTest.java index e7f3f986752..e657c568d7d 100644 --- a/ppl/src/test/java/org/opensearch/sql/ppl/parser/AstBuilderTest.java +++ b/ppl/src/test/java/org/opensearch/sql/ppl/parser/AstBuilderTest.java @@ -82,6 +82,7 @@ import org.opensearch.sql.ast.tree.ML; import org.opensearch.sql.ast.tree.RareTopN.CommandType; import org.opensearch.sql.common.antlr.SyntaxCheckException; +import org.opensearch.sql.common.error.ErrorReport; import org.opensearch.sql.common.setting.Settings; import org.opensearch.sql.common.setting.Settings.Key; import org.opensearch.sql.exception.SemanticCheckException; @@ -409,7 +410,7 @@ public void testStatsCommandWithSpan() { defaultStatsArgs())); } - @Test(expected = org.opensearch.sql.common.antlr.SyntaxCheckException.class) + @Test(expected = org.opensearch.sql.common.error.ErrorReport.class) public void throwExceptionWithEmptyGroupByList() { plan("source=t | stats avg(price) by)"); } @@ -1649,10 +1650,9 @@ public void testMvmapWithLambdaSecondArgThrowsException() { @Test public void testMvmapWithWrongNumberOfArgsThrowsException() { // Grammar enforces exactly 2 arguments, so parser throws syntax error - assertThrows(SyntaxCheckException.class, () -> plan("source=t | eval result = mvmap(arr)")); + assertThrows(ErrorReport.class, () -> plan("source=t | eval result = mvmap(arr)")); assertThrows( - SyntaxCheckException.class, - () -> plan("source=t | eval result = mvmap(arr, arr * 10, extra)")); + ErrorReport.class, () -> plan("source=t | eval result = mvmap(arr, arr * 10, extra)")); } @Test @@ -1714,20 +1714,20 @@ public void testGraphLookupCommand() { .direction(GraphLookup.Direction.BI) .build()); - // Error: missing edge - SyntaxCheckException from grammar + // Error: missing edge - ErrorReport from grammar assertThrows( - SyntaxCheckException.class, + ErrorReport.class, () -> plan("source=t | graphLookup employees start=id as" + " reportingHierarchy")); - // Error: missing lookup table - SyntaxCheckException from grammar + // Error: missing lookup table - ErrorReport from grammar assertThrows( - SyntaxCheckException.class, + ErrorReport.class, () -> plan("source=t | graphLookup start=id edge=manager-->name as" + " reportingHierarchy")); - // Error: missing start - SyntaxCheckException from grammar + // Error: missing start - ErrorReport from grammar assertThrows( - SyntaxCheckException.class, + ErrorReport.class, () -> plan("source=t | graphLookup employees edge=manager-->name as reportingHierarchy")); // graphLookup with hyphenated fromField (space before arrow) @@ -1824,7 +1824,7 @@ public void testEmptyPipeAndTrailingPipeTogether() { * Tests that the parser correctly rejects queries with invalid command tokens after a pipe, * ensuring proper error detection for malformed queries. */ - @Test(expected = org.opensearch.sql.common.antlr.SyntaxCheckException.class) + @Test(expected = org.opensearch.sql.common.error.ErrorReport.class) public void testMalformedPipeProducesSyntaxError() { plan("source=t | invalidCmd |"); } diff --git a/ppl/src/test/java/org/opensearch/sql/ppl/parser/AstExpressionBuilderTest.java b/ppl/src/test/java/org/opensearch/sql/ppl/parser/AstExpressionBuilderTest.java index ce7a120ff56..20889f0a5ca 100644 --- a/ppl/src/test/java/org/opensearch/sql/ppl/parser/AstExpressionBuilderTest.java +++ b/ppl/src/test/java/org/opensearch/sql/ppl/parser/AstExpressionBuilderTest.java @@ -66,6 +66,7 @@ import org.opensearch.sql.ast.tree.Chart; import org.opensearch.sql.calcite.plan.OpenSearchConstants; import org.opensearch.sql.common.antlr.SyntaxCheckException; +import org.opensearch.sql.common.error.ErrorReport; import org.opensearch.sql.exception.SemanticCheckException; public class AstExpressionBuilderTest extends AstBuilderTest { @@ -1429,8 +1430,7 @@ public void testPercentileShortcutFunctionsBoundaryValues() { @Test public void testPercentileShortcutFunctionInvalidNegativeValue() { - assertThrows( - SyntaxCheckException.class, () -> assertEqual("source=t | stats perc-1(a)", (Node) null)); + assertThrows(ErrorReport.class, () -> assertEqual("source=t | stats perc-1(a)", (Node) null)); } @Test @@ -1630,7 +1630,7 @@ public void testTimechartInvalidUseOtherValue() { @Test public void testTimechartInvalidParameter() { assertThrows( - SyntaxCheckException.class, + ErrorReport.class, () -> assertEqual("source=t | timechart invalidparam=value count()", (Node) null)); } diff --git a/sql/src/test/java/org/opensearch/sql/common/antlr/SyntaxParserTestBase.java b/sql/src/test/java/org/opensearch/sql/common/antlr/SyntaxParserTestBase.java index 1a2e9518ef0..7ab4d01bc1d 100644 --- a/sql/src/test/java/org/opensearch/sql/common/antlr/SyntaxParserTestBase.java +++ b/sql/src/test/java/org/opensearch/sql/common/antlr/SyntaxParserTestBase.java @@ -11,6 +11,7 @@ import lombok.AccessLevel; import lombok.Getter; import lombok.RequiredArgsConstructor; +import org.opensearch.sql.common.error.ErrorReport; /** A base class for tests for SQL or PPL parser. */ @RequiredArgsConstructor(access = AccessLevel.PROTECTED) @@ -32,6 +33,6 @@ protected void acceptQuery(String query) { * @param query Query to test. */ protected void rejectQuery(String query) { - assertThrows(SyntaxCheckException.class, () -> parser.parse(query)); + assertThrows(ErrorReport.class, () -> parser.parse(query)); } } diff --git a/sql/src/test/java/org/opensearch/sql/sql/antlr/SQLSyntaxParserTest.java b/sql/src/test/java/org/opensearch/sql/sql/antlr/SQLSyntaxParserTest.java index c43044508b8..41170b85884 100644 --- a/sql/src/test/java/org/opensearch/sql/sql/antlr/SQLSyntaxParserTest.java +++ b/sql/src/test/java/org/opensearch/sql/sql/antlr/SQLSyntaxParserTest.java @@ -8,6 +8,7 @@ import static org.junit.jupiter.api.Assertions.assertAll; import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; import com.google.common.collect.ImmutableMap; import com.google.common.collect.Streams; @@ -23,7 +24,7 @@ import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.Arguments; import org.junit.jupiter.params.provider.MethodSource; -import org.opensearch.sql.common.antlr.SyntaxCheckException; +import org.opensearch.sql.common.error.ErrorReport; class SQLSyntaxParserTest { @@ -72,7 +73,7 @@ public void canParseHiddenIndexName() { @Test public void canNotParseIndexNameWithSpecialChar() { - assertThrows(SyntaxCheckException.class, () -> parser.parse("SELECT * FROM hello+world")); + assertThrows(ErrorReport.class, () -> parser.parse("SELECT * FROM hello+world")); } @Test @@ -82,12 +83,12 @@ public void canParseIndexNameWithSpecialCharQuoted() { @Test public void canNotParseIndexNameStartingWithNumber() { - assertThrows(SyntaxCheckException.class, () -> parser.parse("SELECT * FROM 123test")); + assertThrows(ErrorReport.class, () -> parser.parse("SELECT * FROM 123test")); } @Test public void canNotParseIndexNameSingleQuoted() { - assertThrows(SyntaxCheckException.class, () -> parser.parse("SELECT * FROM 'test'")); + assertThrows(ErrorReport.class, () -> parser.parse("SELECT * FROM 'test'")); } @Test @@ -136,10 +137,10 @@ public void canParseCaseStatement() { @Test public void canNotParseAggregateFunctionWithWrongArgument() { - assertThrows(SyntaxCheckException.class, () -> parser.parse("SELECT SUM() FROM test")); - assertThrows(SyntaxCheckException.class, () -> parser.parse("SELECT AVG() FROM test")); - assertThrows(SyntaxCheckException.class, () -> parser.parse("SELECT SUM(a,b) FROM test")); - assertThrows(SyntaxCheckException.class, () -> parser.parse("SELECT AVG(a,b,c) FROM test")); + assertThrows(ErrorReport.class, () -> parser.parse("SELECT SUM() FROM test")); + assertThrows(ErrorReport.class, () -> parser.parse("SELECT AVG() FROM test")); + assertThrows(ErrorReport.class, () -> parser.parse("SELECT SUM(a,b) FROM test")); + assertThrows(ErrorReport.class, () -> parser.parse("SELECT AVG(a,b,c) FROM test")); } @Test @@ -155,7 +156,7 @@ public void canParseOrderByClause() { @Test public void canNotParseShowStatementWithoutFilterClause() { - assertThrows(SyntaxCheckException.class, () -> parser.parse("SHOW TABLES")); + assertThrows(ErrorReport.class, () -> parser.parse("SHOW TABLES")); } private static Stream nowLikeFunctionsData() { @@ -212,7 +213,7 @@ private static Stream getInvalidPartForExtractFunction() { @MethodSource("getInvalidPartForExtractFunction") public void cannot_parse_extract_function_invalid_part(String part) { assertThrows( - SyntaxCheckException.class, + ErrorReport.class, () -> parser.parse(String.format("SELECT extract(%s FROM \"2023-02-06\")", part))); } @@ -266,8 +267,7 @@ public void can_parse_get_format_function(String type, String format) { @Test public void cannot_parse_get_format_function_with_bad_arg() { - assertThrows( - SyntaxCheckException.class, () -> parser.parse("GET_FORMAT(NONSENSE_ARG,'INTERNAL')")); + assertThrows(ErrorReport.class, () -> parser.parse("GET_FORMAT(NONSENSE_ARG,'INTERNAL')")); } @Test @@ -630,22 +630,16 @@ public void can_parse_yearweek_function() { @Test public void describe_request_accepts_only_quoted_string_literals() { assertAll( + () -> assertThrows(ErrorReport.class, () -> parser.parse("DESCRIBE TABLES LIKE bank")), + () -> assertThrows(ErrorReport.class, () -> parser.parse("DESCRIBE TABLES LIKE %bank%")), + () -> assertThrows(ErrorReport.class, () -> parser.parse("DESCRIBE TABLES LIKE `bank`")), () -> assertThrows( - SyntaxCheckException.class, () -> parser.parse("DESCRIBE TABLES LIKE bank")), - () -> - assertThrows( - SyntaxCheckException.class, () -> parser.parse("DESCRIBE TABLES LIKE %bank%")), - () -> - assertThrows( - SyntaxCheckException.class, () -> parser.parse("DESCRIBE TABLES LIKE `bank`")), - () -> - assertThrows( - SyntaxCheckException.class, + ErrorReport.class, () -> parser.parse("DESCRIBE TABLES LIKE %bank% COLUMNS LIKE %status%")), () -> assertThrows( - SyntaxCheckException.class, + ErrorReport.class, () -> parser.parse("DESCRIBE TABLES LIKE 'bank' COLUMNS LIKE status")), () -> assertNotNull(parser.parse("DESCRIBE TABLES LIKE 'bank' COLUMNS LIKE \"status\"")), () -> assertNotNull(parser.parse("DESCRIBE TABLES LIKE \"bank\" COLUMNS LIKE 'status'"))); @@ -654,11 +648,9 @@ public void describe_request_accepts_only_quoted_string_literals() { @Test public void show_request_accepts_only_quoted_string_literals() { assertAll( - () -> assertThrows(SyntaxCheckException.class, () -> parser.parse("SHOW TABLES LIKE bank")), - () -> - assertThrows(SyntaxCheckException.class, () -> parser.parse("SHOW TABLES LIKE %bank%")), - () -> - assertThrows(SyntaxCheckException.class, () -> parser.parse("SHOW TABLES LIKE `bank`")), + () -> assertThrows(ErrorReport.class, () -> parser.parse("SHOW TABLES LIKE bank")), + () -> assertThrows(ErrorReport.class, () -> parser.parse("SHOW TABLES LIKE %bank%")), + () -> assertThrows(ErrorReport.class, () -> parser.parse("SHOW TABLES LIKE `bank`")), () -> assertNotNull(parser.parse("SHOW TABLES LIKE 'bank'")), () -> assertNotNull(parser.parse("SHOW TABLES LIKE \"bank\""))); } @@ -848,4 +840,13 @@ public String next() { var it = new QueryGenerator(); return Streams.stream(it); } + + @Test + public void testUnmatchedParenthesesProvidesSuggestion() { + ErrorReport error = assertThrows(ErrorReport.class, () -> parser.parse("SELECT (1 + 2")); + + assertNotNull(error.getSuggestion()); + assertTrue(error.getSuggestion().contains("Missing closing parenthesis ')'")); + assertTrue(error.getSuggestion().contains("Check that all parentheses are balanced")); + } } diff --git a/sql/src/test/java/org/opensearch/sql/sql/antlr/suggestion/ContextFactory.java b/sql/src/test/java/org/opensearch/sql/sql/antlr/suggestion/ContextFactory.java new file mode 100644 index 00000000000..a054e8558e2 --- /dev/null +++ b/sql/src/test/java/org/opensearch/sql/sql/antlr/suggestion/ContextFactory.java @@ -0,0 +1,61 @@ +/* + * Copyright OpenSearch Contributors + * SPDX-License-Identifier: Apache-2.0 + */ + +package org.opensearch.sql.sql.antlr.suggestion; + +import org.antlr.v4.runtime.BaseErrorListener; +import org.antlr.v4.runtime.CommonTokenStream; +import org.antlr.v4.runtime.RecognitionException; +import org.antlr.v4.runtime.Recognizer; +import org.antlr.v4.runtime.Token; +import org.opensearch.sql.common.antlr.CaseInsensitiveCharStream; +import org.opensearch.sql.common.antlr.suggestion.SyntaxErrorContext; +import org.opensearch.sql.sql.antlr.parser.OpenSearchSQLLexer; +import org.opensearch.sql.sql.antlr.parser.OpenSearchSQLParser; + +/** Runs the SQL parser on a query and returns the SyntaxErrorContext from the first error. */ +final class ContextFactory { + private ContextFactory() {} + + static SyntaxErrorContext contextFor(String query) { + OpenSearchSQLLexer lexer = new OpenSearchSQLLexer(new CaseInsensitiveCharStream(query)); + CommonTokenStream tokens = new CommonTokenStream(lexer); + OpenSearchSQLParser parser = new OpenSearchSQLParser(tokens); + parser.removeErrorListeners(); + Capturing capturing = new Capturing(); + parser.addErrorListener(capturing); + try { + parser.root(); + } catch (RuntimeException ignored) { + // some errors bail out of the parse + } + if (capturing.context == null) { + throw new AssertionError("expected a syntax error for query: " + query); + } + return capturing.context; + } + + private static class Capturing extends BaseErrorListener { + SyntaxErrorContext context; + + @Override + public void syntaxError( + Recognizer recognizer, + Object offendingSymbol, + int line, + int charPositionInLine, + String msg, + RecognitionException e) { + if (context != null) return; + context = + new SyntaxErrorContext( + recognizer, + (Token) offendingSymbol, + (CommonTokenStream) recognizer.getInputStream(), + ((CommonTokenStream) recognizer.getInputStream()).getText(), + e); + } + } +} diff --git a/sql/src/test/java/org/opensearch/sql/sql/antlr/suggestion/ExpectedTokensSuggestionProviderTest.java b/sql/src/test/java/org/opensearch/sql/sql/antlr/suggestion/ExpectedTokensSuggestionProviderTest.java new file mode 100644 index 00000000000..b007d059822 --- /dev/null +++ b/sql/src/test/java/org/opensearch/sql/sql/antlr/suggestion/ExpectedTokensSuggestionProviderTest.java @@ -0,0 +1,38 @@ +/* + * Copyright OpenSearch Contributors + * SPDX-License-Identifier: Apache-2.0 + */ + +package org.opensearch.sql.sql.antlr.suggestion; + +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.util.Optional; +import org.junit.jupiter.api.Test; +import org.opensearch.sql.common.antlr.suggestion.ExpectedTokensSuggestionProvider; + +class ExpectedTokensSuggestionProviderTest { + private final ExpectedTokensSuggestionProvider provider = new ExpectedTokensSuggestionProvider(); + + @Test + void returnsFallbackSuggestionWithExpectedTokens() { + // SELECT * FROM has a RecognitionException with a populated expected set. + Optional suggestion = + provider.getSuggestion(ContextFactory.contextFor("SELECT * FROM")); + assertTrue(suggestion.isPresent()); + String suggestionText = suggestion.get(); + assertTrue( + suggestionText.startsWith("Expected tokens:") + || suggestionText.startsWith("Expected one of "), + "got: " + suggestionText); + } + + @Test + void returnsEmptyWhenNoRecognitionException() { + // Parser-recovered errors have no RecognitionException: the provider must no-op, not crash. + Optional suggestion = + provider.getSuggestion(ContextFactory.contextFor("SELECT FROM t")); + // Either empty, or populated - neither should throw. We just assert it doesn't throw. + assertTrue(suggestion != null); + } +} diff --git a/sql/src/test/java/org/opensearch/sql/sql/antlr/suggestion/SyntaxErrorSuggestionRegistryTest.java b/sql/src/test/java/org/opensearch/sql/sql/antlr/suggestion/SyntaxErrorSuggestionRegistryTest.java new file mode 100644 index 00000000000..e4e9477cd54 --- /dev/null +++ b/sql/src/test/java/org/opensearch/sql/sql/antlr/suggestion/SyntaxErrorSuggestionRegistryTest.java @@ -0,0 +1,53 @@ +/* + * Copyright OpenSearch Contributors + * SPDX-License-Identifier: Apache-2.0 + */ + +package org.opensearch.sql.sql.antlr.suggestion; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.util.Optional; +import org.junit.jupiter.api.Test; +import org.opensearch.sql.common.antlr.suggestion.SyntaxErrorContext; +import org.opensearch.sql.common.antlr.suggestion.SyntaxErrorSuggestionProvider; +import org.opensearch.sql.common.antlr.suggestion.SyntaxErrorSuggestionRegistry; + +class SyntaxErrorSuggestionRegistryTest { + + /** Lower priority value must win over a higher-priority-number provider on the same match. */ + @Test + void lowerPriorityProviderWinsOverHigherPriorityProvider() { + StubProvider lowPrioritySuggestion = new StubProvider("low-wins", 1); + StubProvider highPrioritySuggestion = new StubProvider("high-loses", Integer.MAX_VALUE - 1); + SyntaxErrorSuggestionRegistry.register(highPrioritySuggestion, lowPrioritySuggestion); + + // Provide a context that both will match (both stubs ignore the context). + SyntaxErrorContext ctx = ContextFactory.contextFor("SELECT FROM t"); + Optional suggestion = SyntaxErrorSuggestionRegistry.findSuggestion(ctx); + + assertTrue(suggestion.isPresent()); + assertEquals("low-wins", suggestion.get()); + } + + private static class StubProvider implements SyntaxErrorSuggestionProvider { + private final String suggestion; + private final int priority; + + StubProvider(String suggestion, int priority) { + this.suggestion = suggestion; + this.priority = priority; + } + + @Override + public Optional getSuggestion(SyntaxErrorContext context) { + return Optional.of(suggestion); + } + + @Override + public int getPriority() { + return priority; + } + } +}