Re: [PR] fix: Improve reserved keyword SQL parse errors (druid)

["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