You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by "FrankChen021 (via GitHub)" <gi...@apache.org> on 2026/07/22 07:29:34 UTC
[PR] Improve reserved keyword SQL parse errors (druid)
FrankChen021 opened a new pull request, #19719:
URL: https://github.com/apache/druid/pull/19719
## Summary
- Detect reserved Calcite tokens in Druid SQL parse errors.
- Tell users how to quote the token as a Druid identifier.
- Add coverage for SELECT start FROM sys."segments" LIMIT 1.
## Root cause
Calcite rejects reserved tokens during parsing, before Druid can resolve them as columns or datasources. Its generic error listed a large set of parser alternatives without explaining the identifier-quoting remedy.
## User impact
Users with a column or datasource whose name is a reserved keyword receive a concise, actionable error that includes the quoted identifier form.
## Validation
- mvn -pl sql checkstyle:check
- Focused DruidSqlParserTest not run locally: this branch requires Java 25 and the available compiler is Java 21.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] fix: Improve reserved keyword SQL parse errors (druid)
Posted by "FrankChen021 (via GitHub)" <gi...@apache.org>.
FrankChen021 commented on code in PR #19719:
URL: https://github.com/apache/druid/pull/19719#discussion_r3662302887
##########
sql/src/main/java/org/apache/druid/sql/calcite/parser/DruidSqlParser.java:
##########
@@ -232,6 +259,29 @@ private static DruidException translateParseException(SqlParseException e)
return InvalidSqlInput.exception(e.getMessage());
}
+ private static boolean isIdentifierExpected(String[] tokenDictionary, int[][] expectedTokenSequences)
+ {
+ for (int[] expectedTokenSequence : expectedTokenSequences) {
+ if (expectedTokenSequence.length > 0) {
+ final String token = tokenDictionary[expectedTokenSequence[0]];
+ if ("<IDENTIFIER>".equals(token)
+ || "<QUOTED_IDENTIFIER>".equals(token)
+ || "<BACK_QUOTED_IDENTIFIER>".equals(token)
+ || "<BRACKET_QUOTED_IDENTIFIER>".equals(token)
+ || "<UNICODE_QUOTED_IDENTIFIER>".equals(token)) {
+ return true;
+ }
+ }
+ }
+ return false;
+ }
+
+ private static boolean isFunctionCall(ParseException parseException)
+ {
+ final Token nextToken = parseException.currentToken.next;
+ return nextToken.next != null && "(".equals(nextToken.next.image);
Review Comment:
Fixed in 584c521e9a. Function-call context is now determined from the original SQL at Calcite's failure position, rather than accessing an unfetched JavaCC lookahead token.
##########
sql/src/test/java/org/apache/druid/sql/calcite/parser/DruidSqlParserTest.java:
##########
@@ -140,4 +140,44 @@ public void test_sqlLiteralToContextValue_unsupportedType()
);
Assert.assertTrue(exception.getMessage().contains("Unsupported type for SET"));
}
+
+ @Test
+ public void testParse_reservedKeywordIdentifier()
+ {
+ final DruidException exception = Assert.assertThrows(
+ DruidException.class,
+ () -> DruidSqlParser.parse("SELECT start FROM sys.\"segments\" LIMIT 1", false)
+ );
+
+ Assert.assertEquals(
+ "Token [start] (line [1], column [8]) is a reserved keyword. "
+ + "To use it as an identifier, quote it as [\"start\"]",
+ exception.getMessage()
+ );
+ }
+
+ @Test
+ public void testParse_reservedKeywordOutsideIdentifierContext()
+ {
+ final DruidException exception = Assert.assertThrows(
+ DruidException.class,
+ () -> DruidSqlParser.parse("SELECT * FROM foo GROUP ORDER BY x", false)
+ );
+
+ Assert.assertTrue(exception.getMessage().contains("Received an unexpected token"));
Review Comment:
Fixed in 584c521e9a. The test now asserts only that the specialized reserved-keyword hint is absent, without depending on Calcite diagnostic wording.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] fix: Improve reserved keyword SQL parse errors (druid)
Posted by "FrankChen021 (via GitHub)" <gi...@apache.org>.
FrankChen021 commented on code in PR #19719:
URL: https://github.com/apache/druid/pull/19719#discussion_r3657349251
##########
sql/src/test/java/org/apache/druid/sql/calcite/parser/DruidSqlParserTest.java:
##########
@@ -140,4 +140,44 @@ public void test_sqlLiteralToContextValue_unsupportedType()
);
Assert.assertTrue(exception.getMessage().contains("Unsupported type for SET"));
}
+
+ @Test
+ public void testParse_reservedKeywordIdentifier()
+ {
+ final DruidException exception = Assert.assertThrows(
+ DruidException.class,
+ () -> DruidSqlParser.parse("SELECT start FROM sys.\"segments\" LIMIT 1", false)
+ );
+
+ Assert.assertEquals(
+ "Token [start] (line [1], column [8]) is a reserved keyword. "
+ + "To use it as an identifier, quote it as [\"start\"]",
+ exception.getMessage()
+ );
+ }
+
+ @Test
+ public void testParse_reservedKeywordOutsideIdentifierContext()
+ {
+ final DruidException exception = Assert.assertThrows(
+ DruidException.class,
+ () -> DruidSqlParser.parse("SELECT * FROM foo GROUP ORDER BY x", false)
+ );
+
+ Assert.assertTrue(exception.getMessage().contains("Received an unexpected token"));
Review Comment:
[P1] Assert the existing normalized diagnostic
Uppercase `ORDER` follows Calcite's `Incorrect syntax near the keyword` path, so this `Received an unexpected token` assertion fails. Assert only that the reserved-keyword hint is absent, or match the normalized diagnostic.
##########
sql/src/main/java/org/apache/druid/sql/calcite/parser/DruidSqlParser.java:
##########
@@ -232,6 +259,29 @@ private static DruidException translateParseException(SqlParseException e)
return InvalidSqlInput.exception(e.getMessage());
}
+ private static boolean isIdentifierExpected(String[] tokenDictionary, int[][] expectedTokenSequences)
+ {
+ for (int[] expectedTokenSequence : expectedTokenSequences) {
+ if (expectedTokenSequence.length > 0) {
+ final String token = tokenDictionary[expectedTokenSequence[0]];
+ if ("<IDENTIFIER>".equals(token)
+ || "<QUOTED_IDENTIFIER>".equals(token)
+ || "<BACK_QUOTED_IDENTIFIER>".equals(token)
+ || "<BRACKET_QUOTED_IDENTIFIER>".equals(token)
+ || "<UNICODE_QUOTED_IDENTIFIER>".equals(token)) {
+ return true;
+ }
+ }
+ }
+ return false;
+ }
+
+ private static boolean isFunctionCall(ParseException parseException)
+ {
+ final Token nextToken = parseException.currentToken.next;
+ return nextToken.next != null && "(".equals(nextToken.next.image);
Review Comment:
[P1] Do not rely on unfetched token lookahead
`nextToken.next` is not guaranteed to exist after JavaCC stops at the unexpected token. Consequently `strlen(unnest(...))` still receives the reserved-identifier hint. The new unit test fails, and QTest reports the same regression across three suppliers. Determine function-call context without relying on an unfetched token.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Improve reserved keyword SQL parse errors (druid)
Posted by "FrankChen021 (via GitHub)" <gi...@apache.org>.
FrankChen021 commented on code in PR #19719:
URL: https://github.com/apache/druid/pull/19719#discussion_r3644309593
##########
sql/src/main/java/org/apache/druid/sql/calcite/parser/DruidSqlParser.java:
##########
@@ -199,6 +204,24 @@ private static DruidException translateParseException(SqlParseException e)
} else {
final String theUnexpectedToken = getUnexpectedTokenString(parseException);
+ if (parserMetadata.isReservedWord(theUnexpectedToken.toUpperCase(Locale.ROOT))) {
+ return InvalidSqlInput
+ .exception(
+ e,
+ "Token [%s] (line [%s], column [%s]) is a reserved keyword. "
+ + "To use it as an identifier, quote it as [\"%s\"]",
+ theUnexpectedToken,
+ failurePosition.getLineNum(),
+ failurePosition.getColumnNum(),
+ theUnexpectedToken
+ )
+ .withContext("line", failurePosition.getLineNum())
+ .withContext("column", failurePosition.getColumnNum())
+ .withContext("endLine", failurePosition.getEndLineNum())
+ .withContext("endColumn", failurePosition.getEndColumnNum())
+ .withContext("token", theUnexpectedToken);
+ }
+
final String[] tokenDictionary = e.getTokenImages();
final int[][] expectedTokenSequences = e.getExpectedTokenSequences();
final ArrayList<String> expectedTokens = new ArrayList<>(expectedTokenSequences.length);
Review Comment:
Fixed in a40f7d4559. The specialized hint now requires an identifier token to be expected at the current parse position, includes the defensive metadata null check, and retains the generic parser diagnostic otherwise.
##########
sql/src/main/java/org/apache/druid/sql/calcite/parser/DruidSqlParser.java:
##########
@@ -199,6 +204,24 @@ private static DruidException translateParseException(SqlParseException e)
} else {
final String theUnexpectedToken = getUnexpectedTokenString(parseException);
+ if (parserMetadata.isReservedWord(theUnexpectedToken.toUpperCase(Locale.ROOT))) {
Review Comment:
Fixed in a40f7d4559. The hint is now gated on identifier context, and a regression test verifies that SELECT * FROM foo GROUP ORDER BY x retains the normal unexpected-token error for ORDER.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Improve reserved keyword SQL parse errors (druid)
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #19719:
URL: https://github.com/apache/druid/pull/19719#discussion_r3628426747
##########
sql/src/main/java/org/apache/druid/sql/calcite/parser/DruidSqlParser.java:
##########
@@ -199,6 +204,24 @@ private static DruidException translateParseException(SqlParseException e)
} else {
final String theUnexpectedToken = getUnexpectedTokenString(parseException);
+ if (parserMetadata.isReservedWord(theUnexpectedToken.toUpperCase(Locale.ROOT))) {
+ return InvalidSqlInput
+ .exception(
+ e,
+ "Token [%s] (line [%s], column [%s]) is a reserved keyword. "
+ + "To use it as an identifier, quote it as [\"%s\"]",
+ theUnexpectedToken,
+ failurePosition.getLineNum(),
+ failurePosition.getColumnNum(),
+ theUnexpectedToken
+ )
+ .withContext("line", failurePosition.getLineNum())
+ .withContext("column", failurePosition.getColumnNum())
+ .withContext("endLine", failurePosition.getEndLineNum())
+ .withContext("endColumn", failurePosition.getEndColumnNum())
+ .withContext("token", theUnexpectedToken);
+ }
+
final String[] tokenDictionary = e.getTokenImages();
final int[][] expectedTokenSequences = e.getExpectedTokenSequences();
final ArrayList<String> expectedTokens = new ArrayList<>(expectedTokenSequences.length);
Review Comment:
The reserved-keyword hint is currently triggered for *any* parse error where the unexpected token happens to be a reserved word (e.g., `FROM`, `SELECT`, etc.), even when the parser was not expecting an identifier. This can replace a correct syntax error with a misleading “quote it as an identifier” message. Consider gating the hint on whether the expected tokens include identifier forms (e.g., `<IDENTIFIER>`, `<QUOTED_IDENTIFIER>`, etc.), and also guard against `parserMetadata` being null.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] fix: Improve reserved keyword SQL parse errors (druid)
Posted by "AlbericByte (via GitHub)" <gi...@apache.org>.
AlbericByte merged PR #19719:
URL: https://github.com/apache/druid/pull/19719
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Improve reserved keyword SQL parse errors (druid)
Posted by "FrankChen021 (via GitHub)" <gi...@apache.org>.
FrankChen021 commented on code in PR #19719:
URL: https://github.com/apache/druid/pull/19719#discussion_r3630300827
##########
sql/src/main/java/org/apache/druid/sql/calcite/parser/DruidSqlParser.java:
##########
@@ -199,6 +204,24 @@ private static DruidException translateParseException(SqlParseException e)
} else {
final String theUnexpectedToken = getUnexpectedTokenString(parseException);
+ if (parserMetadata.isReservedWord(theUnexpectedToken.toUpperCase(Locale.ROOT))) {
Review Comment:
[P2] Gate the reserved-word hint on identifier context
This treats any parse failure whose current token happens to be reserved as an identifier error, even when the grammar expected a different keyword. For example, `SELECT * FROM foo GROUP ORDER BY x` is missing `BY`, but the new branch tells the user to quote `ORDER`, obscuring the actual syntax error. Please gate the specialized hint on parser expectations that permit an identifier at this position (and retain the normal expected-token diagnostic otherwise), with a regression test for a reserved keyword encountered in non-identifier context.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] fix: Improve reserved keyword SQL parse errors (druid)
Posted by "FrankChen021 (via GitHub)" <gi...@apache.org>.
FrankChen021 commented on code in PR #19719:
URL: https://github.com/apache/druid/pull/19719#discussion_r3665456837
##########
sql/src/main/java/org/apache/druid/sql/calcite/parser/DruidSqlParser.java:
##########
@@ -232,6 +262,57 @@ private static DruidException translateParseException(SqlParseException e)
return InvalidSqlInput.exception(e.getMessage());
}
+ private static boolean isIdentifierExpected(String[] tokenDictionary, int[][] expectedTokenSequences)
+ {
+ for (int[] expectedTokenSequence : expectedTokenSequences) {
+ if (expectedTokenSequence.length > 0) {
+ final String token = tokenDictionary[expectedTokenSequence[0]];
+ if ("<IDENTIFIER>".equals(token)
+ || "<QUOTED_IDENTIFIER>".equals(token)
+ || "<BACK_QUOTED_IDENTIFIER>".equals(token)
+ || "<BRACKET_QUOTED_IDENTIFIER>".equals(token)
+ || "<UNICODE_QUOTED_IDENTIFIER>".equals(token)) {
+ return true;
+ }
+ }
+ }
+ return false;
+ }
+
+ private static boolean isKeywordExpected(
+ String unexpectedToken,
+ String[] tokenDictionary,
+ int[][] expectedTokenSequences
+ )
+ {
+ for (int[] expectedTokenSequence : expectedTokenSequences) {
+ if (expectedTokenSequence.length > 0
+ && unexpectedToken.equalsIgnoreCase(
+ SqlParserUtil.getTokenVal(tokenDictionary[expectedTokenSequence[0]])
+ )) {
+ return true;
+ }
+ }
+ return false;
+ }
+
+ private static boolean isFunctionCall(String sql, SqlParserPos failurePosition, String token)
+ {
+ int tokenEndOffset = 0;
+ for (int line = 1; line < failurePosition.getLineNum(); line++) {
+ tokenEndOffset = sql.indexOf('\n', tokenEndOffset) + 1;
+ if (tokenEndOffset == 0) {
+ return false;
+ }
+ }
+
+ tokenEndOffset += failurePosition.getColumnNum() - 1 + token.length();
+ while (tokenEndOffset < sql.length() && Character.isWhitespace(sql.charAt(tokenEndOffset))) {
Review Comment:
[P2] Skip SQL comments when detecting function calls
`isFunctionCall` skips only whitespace, but SQL comments are also valid separators before `(`. For example, `SELECT strlen(unnest /* comment */ (a_int))` now reports that `unnest` is a reserved identifier and recommends quoting it, despite it being used in a function-call context. Skip line/block comments or use lexer token boundaries, and add regression coverage.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org