Skip to content

Commit 3bab06d

Browse files
SwiddisSwiddis
authored
Clean up syntax error reporting (#3278)
* Clean up syntax error reporting Signed-off-by: Simeon Widdis <sawiddis@amazon.com> * Reuse contextStartIndex variable Signed-off-by: Simeon Widdis <sawiddis@amazon.com> * Update tests for new error message Signed-off-by: Simeon Widdis <sawiddis@amazon.com> * Update a comment Signed-off-by: Simeon Widdis <sawiddis@amazon.com> * Simplify details construction Signed-off-by: Simeon Widdis <sawiddis@amazon.com> * Apply feedback: clean up suggestion extraction Signed-off-by: Simeon Widdis <sawiddis@amazon.com> * Apply feedback: clean up suggestion extraction Signed-off-by: Simeon Widdis <sawiddis@amazon.com> * Apply feedback: move exception fragment to const Signed-off-by: Simeon Widdis <sawiddis@amazon.com> * Apply spotless Signed-off-by: Simeon Widdis <sawiddis@amazon.com> * Remove weird extra reports Signed-off-by: Simeon Widdis <sawiddis@amazon.com> * Add some exact error output tests + fix bug Signed-off-by: Simeon Widdis <sawiddis@amazon.com> * Fix the flaky tests I just introduced Signed-off-by: Simeon Widdis <sawiddis@amazon.com> --------- Signed-off-by: Simeon Widdis <sawiddis@amazon.com>
1 parent f5e7869 commit 3bab06d

File tree

7 files changed

+86
-33
lines changed

7 files changed

+86
-33
lines changed

‎common/src/main/java/org/opensearch/sql/common/antlr/SyntaxAnalysisErrorListener.java‎

Lines changed: 46 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -5,19 +5,26 @@
55

66
package org.opensearch.sql.common.antlr;
77

8+
import java.util.ArrayList;
9+
import java.util.List;
810
import java.util.Locale;
911
import org.antlr.v4.runtime.BaseErrorListener;
1012
import org.antlr.v4.runtime.CommonTokenStream;
1113
import org.antlr.v4.runtime.RecognitionException;
1214
import org.antlr.v4.runtime.Recognizer;
1315
import org.antlr.v4.runtime.Token;
16+
import org.antlr.v4.runtime.Vocabulary;
1417
import org.antlr.v4.runtime.misc.IntervalSet;
1518

1619
/**
1720
* Syntax analysis error listener that handles any syntax error by throwing exception with useful
1821
* information.
1922
*/
2023
public class SyntaxAnalysisErrorListener extends BaseErrorListener {
24+
// Show up to this many characters before the offending token in the query.
25+
private static final int CONTEXT_TRUNCATION_THRESHOLD = 20;
26+
// Avoid presenting too many alternatives when many are available.
27+
private static final int SUGGESTION_TRUNCATION_THRESHOLD = 5;
2128

2229
@Override
2330
public void syntaxError(
@@ -35,8 +42,7 @@ public void syntaxError(
3542
throw new SyntaxCheckException(
3643
String.format(
3744
Locale.ROOT,
38-
"Failed to parse query due to offending symbol [%s] "
39-
+ "at: '%s' <--- HERE... More details: %s",
45+
"[%s] is not a valid term at this part of the query: '%s' <-- HERE. %s",
4046
getOffendingText(offendingToken),
4147
truncateQueryAtOffendingToken(query, offendingToken),
4248
getDetails(recognizer, msg, e)));
@@ -47,21 +53,47 @@ private String getOffendingText(Token offendingToken) {
4753
}
4854

4955
private String truncateQueryAtOffendingToken(String query, Token offendingToken) {
50-
return query.substring(0, offendingToken.getStopIndex() + 1);
56+
int contextStartIndex = offendingToken.getStartIndex() - CONTEXT_TRUNCATION_THRESHOLD;
57+
if (contextStartIndex < 3) { // The ellipses won't save us anything below the first 4 characters
58+
return query.substring(0, offendingToken.getStopIndex() + 1);
59+
}
60+
return "..." + query.substring(contextStartIndex, offendingToken.getStopIndex() + 1);
61+
}
62+
63+
private List<String> topSuggestions(Recognizer<?, ?> recognizer, IntervalSet continuations) {
64+
Vocabulary vocab = recognizer.getVocabulary();
65+
List<String> tokenNames = new ArrayList<>(SUGGESTION_TRUNCATION_THRESHOLD);
66+
for (int tokenType :
67+
continuations
68+
.toList()
69+
.subList(0, Math.min(continuations.size(), SUGGESTION_TRUNCATION_THRESHOLD))) {
70+
tokenNames.add(vocab.getDisplayName(tokenType));
71+
}
72+
return tokenNames;
5173
}
5274

53-
/**
54-
* As official JavaDoc says, e=null means parser was able to recover from the error. In other
55-
* words, "msg" argument includes the information we want.
56-
*/
57-
private String getDetails(Recognizer<?, ?> recognizer, String msg, RecognitionException e) {
58-
String details;
59-
if (e == null) {
60-
details = msg;
75+
private String getDetails(Recognizer<?, ?> recognizer, String msg, RecognitionException ex) {
76+
if (ex == null) {
77+
// According to the ANTLR docs, ex == null means the parser was able to recover from the
78+
// error.
79+
// In such cases, `msg` includes the raw error information we care about.
80+
return msg;
81+
}
82+
83+
IntervalSet possibleContinuations = ex.getExpectedTokens();
84+
List<String> suggestions = topSuggestions(recognizer, possibleContinuations);
85+
86+
StringBuilder details = new StringBuilder("Expecting ");
87+
if (possibleContinuations.size() > SUGGESTION_TRUNCATION_THRESHOLD) {
88+
details
89+
.append("one of ")
90+
.append(possibleContinuations.size())
91+
.append(" possible tokens. Some examples: ")
92+
.append(String.join(", ", suggestions))
93+
.append(", ...");
6194
} else {
62-
IntervalSet followSet = e.getExpectedTokens();
63-
details = "Expecting tokens in " + followSet.toString(recognizer.getVocabulary());
95+
details.append("tokens: ").append(String.join(", ", suggestions));
6496
}
65-
return details;
97+
return details.toString();
6698
}
6799
}

‎docs/user/dql/troubleshooting.rst‎

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -25,22 +25,22 @@ Query:
2525

2626
.. code-block:: JSON
2727
28-
POST /_plugins/_sql
28+
POST /_plugins/_ppl
2929
{
30-
"query" : "SELECT * FROM sample:data"
30+
"query" : "SOURCE = test_index | where a > 0)"
3131
}
3232
3333
Result:
3434

3535
.. code-block:: JSON
3636
3737
{
38-
"reason": "Invalid SQL query",
39-
"details": "Failed to parse query due to offending symbol [:] at: 'SELECT * FROM xxx WHERE xxx:' <--- HERE...
40-
More details: Expecting tokens in {<EOF>, 'AND', 'BETWEEN', 'GROUP', 'HAVING', 'IN', 'IS', 'LIKE', 'LIMIT',
41-
'NOT', 'OR', 'ORDER', 'REGEXP', '*', '/', '%', '+', '-', 'DIV', 'MOD', '=', '>', '<', '!',
42-
'|', '&', '^', '.', DOT_ID}",
43-
"type": "SyntaxAnalysisException"
38+
"error": {
39+
"reason": "Invalid Query",
40+
"details": "[)] is not a valid term at this part of the query: '..._index | where a > 0)' <-- HERE. extraneous input ')' expecting <EOF>",
41+
"type": "SyntaxCheckException"
42+
},
43+
"status": 400
4444
}
4545
4646
**Workaround**

‎integ-test/src/test/java/org/opensearch/sql/legacy/TestsConstants.java‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -66,4 +66,7 @@ public class TestsConstants {
6666
public static final String DATE_FORMAT = "yyyy-MM-dd'T'HH:mm:ss.SSS'Z'";
6767
public static final String TS_DATE_FORMAT = "yyyy-MM-dd HH:mm:ss.SSS";
6868
public static final String SIMPLE_DATE_FORMAT = "yyyy-MM-dd";
69+
70+
public static final String SYNTAX_EX_MSG_FRAGMENT =
71+
"is not a valid term at this part of the query";
6972
}

‎integ-test/src/test/java/org/opensearch/sql/ppl/DescribeCommandIT.java‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55

66
package org.opensearch.sql.ppl;
77

8+
import static org.opensearch.sql.legacy.TestsConstants.SYNTAX_EX_MSG_FRAGMENT;
89
import static org.opensearch.sql.legacy.TestsConstants.TEST_INDEX_DOG;
910
import static org.opensearch.sql.util.MatcherUtils.columnName;
1011
import static org.opensearch.sql.util.MatcherUtils.verifyColumn;
@@ -80,7 +81,7 @@ public void describeCommandWithoutIndexShouldFailToParse() throws IOException {
8081
fail();
8182
} catch (ResponseException e) {
8283
assertTrue(e.getMessage().contains("RuntimeException"));
83-
assertTrue(e.getMessage().contains("Failed to parse query due to offending symbol"));
84+
assertTrue(e.getMessage().contains(SYNTAX_EX_MSG_FRAGMENT));
8485
}
8586
}
8687
}

‎integ-test/src/test/java/org/opensearch/sql/ppl/QueryAnalysisIT.java‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55

66
package org.opensearch.sql.ppl;
77

8+
import static org.opensearch.sql.legacy.TestsConstants.SYNTAX_EX_MSG_FRAGMENT;
89
import static org.opensearch.sql.legacy.TestsConstants.TEST_INDEX_ACCOUNT;
910

1011
import java.io.IOException;
@@ -14,7 +15,6 @@
1415
import org.opensearch.sql.exception.SemanticCheckException;
1516

1617
public class QueryAnalysisIT extends PPLIntegTestCase {
17-
1818
@Override
1919
public void init() throws IOException {
2020
loadIndex(Index.ACCOUNT);
@@ -82,25 +82,25 @@ public void queryShouldBeCaseInsensitiveInKeywords() {
8282
@Test
8383
public void queryNotStartingWithSearchCommandShouldFailSyntaxCheck() {
8484
String query = "fields firstname";
85-
queryShouldThrowSyntaxException(query, "Failed to parse query due to offending symbol");
85+
queryShouldThrowSyntaxException(query, SYNTAX_EX_MSG_FRAGMENT);
8686
}
8787

8888
@Test
8989
public void queryWithIncorrectCommandShouldFailSyntaxCheck() {
9090
String query = String.format("search source=%s | field firstname", TEST_INDEX_ACCOUNT);
91-
queryShouldThrowSyntaxException(query, "Failed to parse query due to offending symbol");
91+
queryShouldThrowSyntaxException(query, SYNTAX_EX_MSG_FRAGMENT);
9292
}
9393

9494
@Test
9595
public void queryWithIncorrectKeywordsShouldFailSyntaxCheck() {
9696
String query = String.format("search sources=%s", TEST_INDEX_ACCOUNT);
97-
queryShouldThrowSyntaxException(query, "Failed to parse query due to offending symbol");
97+
queryShouldThrowSyntaxException(query, SYNTAX_EX_MSG_FRAGMENT);
9898
}
9999

100100
@Test
101101
public void unsupportedAggregationFunctionShouldFailSyntaxCheck() {
102102
String query = String.format("search source=%s | stats range(age)", TEST_INDEX_ACCOUNT);
103-
queryShouldThrowSyntaxException(query, "Failed to parse query due to offending symbol");
103+
queryShouldThrowSyntaxException(query, SYNTAX_EX_MSG_FRAGMENT);
104104
}
105105

106106
/** Commands that fail semantic analysis should throw {@link SemanticCheckException}. */

‎integ-test/src/test/java/org/opensearch/sql/ppl/SearchCommandIT.java‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -5,8 +5,7 @@
55

66
package org.opensearch.sql.ppl;
77

8-
import static org.opensearch.sql.legacy.TestsConstants.TEST_INDEX_BANK;
9-
import static org.opensearch.sql.legacy.TestsConstants.TEST_INDEX_DOG;
8+
import static org.opensearch.sql.legacy.TestsConstants.*;
109
import static org.opensearch.sql.util.MatcherUtils.columnName;
1110
import static org.opensearch.sql.util.MatcherUtils.rows;
1211
import static org.opensearch.sql.util.MatcherUtils.verifyColumn;
@@ -64,7 +63,7 @@ public void searchCommandWithoutSourceShouldFailToParse() throws IOException {
6463
fail();
6564
} catch (ResponseException e) {
6665
assertTrue(e.getMessage().contains("RuntimeException"));
67-
assertTrue(e.getMessage().contains("Failed to parse query due to offending symbol"));
66+
assertTrue(e.getMessage().contains(SYNTAX_EX_MSG_FRAGMENT));
6867
}
6968
}
7069
}

‎ppl/src/test/java/org/opensearch/sql/ppl/antlr/PPLSyntaxParserTest.java‎

Lines changed: 20 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@
1111

1212
import java.util.List;
1313
import org.antlr.v4.runtime.tree.ParseTree;
14+
import org.hamcrest.text.StringContainsInOrder;
1415
import org.junit.Rule;
1516
import org.junit.Test;
1617
import org.junit.rules.ExpectedException;
@@ -95,7 +96,7 @@ public void testSearchFieldsCommandCrossClusterShouldPass() {
9596
@Test
9697
public void testSearchCommandWithoutSourceShouldFail() {
9798
exceptionRule.expect(RuntimeException.class);
98-
exceptionRule.expectMessage("Failed to parse query due to offending symbol");
99+
exceptionRule.expectMessage("is not a valid term at this part of the query");
99100

100101
new PPLSyntaxParser().parse("search a=1");
101102
}
@@ -337,11 +338,28 @@ public void testDescribeFieldsCommandShouldPass() {
337338
@Test
338339
public void testDescribeCommandWithSourceShouldFail() {
339340
exceptionRule.expect(RuntimeException.class);
340-
exceptionRule.expectMessage("Failed to parse query due to offending symbol");
341+
exceptionRule.expectMessage(
342+
StringContainsInOrder.stringContainsInOrder(
343+
"[=] is not a valid term at this part of the query: 'describe source=' <-- HERE.",
344+
"Expecting tokens:"));
341345

342346
new PPLSyntaxParser().parse("describe source=t");
343347
}
344348

349+
@Test
350+
public void testInvalidOperatorCombinationShouldFail() {
351+
exceptionRule.expect(RuntimeException.class);
352+
exceptionRule.expectMessage(
353+
StringContainsInOrder.stringContainsInOrder(
354+
"[<EOF>] is not a valid term at this part of the query: '...= t | where x > y OR' <--"
355+
+ " HERE.",
356+
"Expecting one of ",
357+
" possible tokens. Some examples: ",
358+
"..."));
359+
360+
new PPLSyntaxParser().parse("source = t | where x > y OR");
361+
}
362+
345363
@Test
346364
public void testCanParseExtractFunction() {
347365
String[] parts =

0 commit comments

Comments
 (0)