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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
