[JDBC-v2] Fix parser to property handle keywords in aliases and table names #2725
Open
[JDBC-v2] Fix parser to property handle keywords in aliases and table names #2725
Conversation
Client V2 CoverageCoverage Report
Class Coverage
|
JDBC V2 CoverageCoverage Report
Class Coverage
|
JDBC V1 CoverageCoverage Report
Class Coverage
|
Client V1 CoverageCoverage Report
Class Coverage
|
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| log.error("Failed to load keywords from " + resource, e); | ||
| } | ||
| return keywords; | ||
| } |
There was a problem hiding this comment.
Resource loading failure breaks keyword alias parsing
High Severity
The loadKeywords() method silently returns an empty set if allowed_keyword_aliases.txt cannot be loaded, causing ALLOWED_ALIAS_KEYWORDS to be empty. This breaks the aliasIdentifier() function at line 931, where isAllowedAlias() will always return false. As a result, valid SQL queries using keywords as aliases will fail to parse. The error is only logged (line 71), making this difficult to diagnose in production when queries that should work start failing.
| } | ||
| Assert.assertEquals(stmt5.getArgCount(), 1, "Should have 1 parameter for: " + sql5); | ||
| if (!stmt2.getTable().equalsIgnoreCase(keyword)) { | ||
| failedKeywords.add(keyword + " (test: INSERT INTO " + keyword + " VALUES (?)) table name check failed"); |
There was a problem hiding this comment.
Test validates wrong statement's table name
Medium Severity
The test validates stmt2.getTable() instead of stmt5.getTable() when checking the INSERT statement at line 887. This checks the table name extracted from the SELECT statement (line 879) instead of the INSERT statement being tested, causing the validation to fail incorrectly when the INSERT statement is tested after the SELECT statement in a different iteration or context where table names might differ.
| BufferedReader reader = new BufferedReader(new InputStreamReader(is, StandardCharsets.UTF_8))) { | ||
|
|
||
| if (is == null) { | ||
| return keywords; |
There was a problem hiding this comment.
Null resource causes NullPointerException in test
Medium Severity
The BufferedReader is created in the try-with-resources declaration using a null InputStream before the null check executes. When the resource file cannot be found, getResourceAsStream() returns null, and new InputStreamReader(is, StandardCharsets.UTF_8) at line 806 throws NullPointerException before reaching the null check at line 808. The test should check for null before creating the InputStreamReader, like the implementation in StatementSQLTest.loadKeywordsFromResource() at lines 116-120.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
INin select statementDESCRIBE (SELECT)Closes #2718
Checklist
Delete items not relevant to your PR:
Note
Medium Risk
Touches core SQL parsing logic and keyword classification, which can change how statements are recognized and parameters/tables extracted; however changes are mostly additive and covered by new tests.
Overview
Improves parser handling of keywords used as identifiers by expanding the ANTLR
keyword/keywordForAliaslists and updating JavaCC alias parsing to only treat a curated allowlist of keywords as valid aliases (loaded from a new resource file).Extends JavaCC statement parsing for additional ClickHouse syntax, including
DESCRIBE (subquery)/DESCRIBE TABLE (subquery),SHOW CHANGED SETTINGS, optional/emptySETTINGSclauses, and more robust array indexing in expressions.Adds new keyword resource lists (allowed alias keywords, allowed table-name keywords, reserved keywords, and a master keyword list) and adds/extends tests to validate keyword-as-table-name/alias behavior and the new statement forms (with one integration keyword probe test added but disabled).
Written by Cursor Bugbot for commit c6a6ad7. This will update automatically on new commits. Configure here.