feat: Add DDL parsers for DROP TABLE, DROP INDEX, and DROP CONSTRAINT statements #216
Conversation
… statements - Implemented DropConstraintParser to parse standalone DROP CONSTRAINT statements. - Implemented DropIndexParser to parse DROP INDEX statements with optional modifiers. - Implemented DropTableParser to parse DROP TABLE statements with optional IF EXISTS and behavior. - Enhanced SqlParser to support parsing of DROP TABLE, DROP INDEX, and DROP CONSTRAINT statements. - Updated SqlPrintTokenParser to handle printing of DROP TABLE, DROP INDEX, and DROP CONSTRAINT statements. - Added tests for DROP TABLE, DROP INDEX, and DROP CONSTRAINT parsing and formatting.
WalkthroughAdds comprehensive DDL support: new DDL AST models, DDL parsers (CREATE INDEX, DROP INDEX, DROP TABLE, ALTER TABLE, DROP CONSTRAINT), extended CREATE TABLE parsing/modeling, printer/token updates, exports, a lexeme-join utility, docs example, and tests; updates SqlParser dispatch and formatting/printing surfaces. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SqlParser
participant Tokenizer as SqlTokenizer
participant DDLParser as DDL-specific Parser
participant Models as AST
User->>SqlParser: parse(sql)
SqlParser->>Tokenizer: tokenize(sql)
Tokenizer-->>SqlParser: lexemes[]
SqlParser->>SqlParser: detect statement kind
alt CREATE INDEX / DROP INDEX / DROP TABLE / ALTER TABLE / DROP CONSTRAINT
SqlParser->>DDLParser: parseFromLexeme(lexemes, idx)
DDLParser->>Models: construct DDL AST
DDLParser-->>SqlParser: ParsedStatement
end
SqlParser-->>User: ParsedStatement
sequenceDiagram
participant Caller
participant PrinterParser as SqlPrintTokenParser
participant Printer as SqlPrinter
participant Models as DDL AST
Caller->>PrinterParser: visit(ddlModel)
PrinterParser->>PrinterParser: build tokens (qualified names, definitions, constraints)
PrinterParser->>Printer: tokens[]
Printer->>Printer: apply indentation (CreateTableDefinition, AlterTableStatement, IndexColumnList)
Printer-->>Caller: formatted SQL
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/core/src/parsers/CreateTableParser.ts (1)
121-126: Potential case-sensitivity bug in CREATE TABLE detection.commandToken is compared verbatim; unlike other parsers, no .toLowerCase() is applied. If the tokenizer preserves source casing, inputs like "CREATE TABLE" will fail.
Apply this minimal fix:
- const commandToken = lexemes[idx].value; - const isTemporary = commandToken === "create temporary table"; - if (commandToken !== "create table" && !isTemporary) { + const commandToken = lexemes[idx].value.toLowerCase(); + const isTemporary = commandToken === "create temporary table"; + if (commandToken !== "create table" && !isTemporary) { throw new Error(`[CreateTableParser] Syntax error at position ${idx}: expected 'CREATE TABLE' but found '${lexemes[idx].value}'.`); }
🧹 Nitpick comments (10)
packages/core/src/parsers/DropTableParser.ts (1)
1-68: Solid DROP TABLE parsing; add position-aware errors (optional)Implementation is correct. Consider enriching errors with lexeme.position to aid debugging.
Example tweak:
- throw new Error(`[DropTableParser] Expected DROP TABLE at index ${idx}.`); + const pos = lexemes[idx]?.position?.startPosition ?? idx; + throw new Error(`[DropTableParser] Expected DROP TABLE at index ${idx} (char ${pos}).`);docs/guide/formatting-recipes.md (1)
180-211: Add missing imports in DDL exampleYou reference DropIndexParser and DropConstraintParser below; include them in the example import for clarity.
-import { - CreateTableParser, - DropTableParser, - CreateIndexParser, - AlterTableParser, - SqlFormatter -} from 'rawsql-ts'; +import { + CreateTableParser, + DropTableParser, + CreateIndexParser, + DropIndexParser, + AlterTableParser, + DropConstraintParser, + SqlFormatter +} from 'rawsql-ts';packages/core/src/parsers/SqlParser.ts (1)
164-182: Dispatch extended for DDL — consider a lookup map (optional)Branching is fine. For maintainability, a token→handler map would reduce cascading if/else as more statements arrive.
// Example shape: const routes = new Map<string, (s: StatementLexemeResult, i:number)=>ParsedStatement>([ ['create index', this.parseCreateIndexStatement], ['create unique index', this.parseCreateIndexStatement], ['drop table', this.parseDropTableStatement], ['drop index', this.parseDropIndexStatement], ['alter table', this.parseAlterTableStatement], ['drop constraint', this.parseDropConstraintStatement], ]);packages/core/src/parsers/DropIndexParser.ts (1)
1-75: Add test case for invalid DROP INDEX modifier orderParser correctly enforces PostgreSQL syntax with CONCURRENTLY before IF EXISTS. Existing test at
packages/core/tests/parsers/DDLParsers.test.ts:50-61validates the correct order. However, test coverage is incomplete—add a case confirming "DROP INDEX IF EXISTS CONCURRENTLY" is rejected (index names are correctly identified):it("rejects DROP INDEX with reversed modifier order", () => { const sql = "DROP INDEX IF EXISTS CONCURRENTLY idx_test"; expect(() => DropIndexParser.parse(sql)).toThrow(); });packages/core/tests/parsers/DDLParsers.test.ts (1)
16-26: Solid end-to-end coverage and formatter use.Good breadth across DROP TABLE/INDEX/CONSTRAINT and ALTER TABLE constraints; comparisons correctly use SqlFormatter. Optionally add a negative case (e.g., DROP TABLE without names) to assert error paths.
As per coding guidelines.
Also applies to: 28-48, 50-60, 62-81, 83-91
packages/core/tests/parsers/CreateTableParser.test.ts (1)
48-50: Tests align with qualified identifiers and richer constraints.Expectations match the formatter’s quoting and constraint rendering. Optionally add a case for CHECK (...) to cover that branch explicitly.
Also applies to: 63-64, 66-97
packages/core/src/parsers/CreateTableParser.ts (1)
263-271: Tighten column identifier parsing (avoid namespaced columns).parseQualifiedName permits dotted identifiers; in column definitions this should be a single identifier. Consider rejecting namespaces here to catch malformed input early.
- const columnNameResult = this.parseQualifiedName(lexemes, idx); + const columnNameResult = this.parseQualifiedName(lexemes, idx); + if (columnNameResult.namespaces && columnNameResult.namespaces.length > 0) { + throw new Error(`[CreateTableParser] Column name must not be qualified at index ${idx}.`); + }packages/core/src/parsers/AlterTableParser.ts (2)
95-106: Narrow ALTER TABLE action dispatch to supported forms.startsWith("add") may incorrectly route "ADD COLUMN" here before throwing later. Match exact tokens to keep intent clear and errors precise.
- if (value.startsWith("add")) { + if (value === "add" || value === "add constraint") { const result = this.parseAddConstraintAction(lexemes, idx); actions.push(result.value); idx = result.newIndex; - } else if (value.startsWith("drop constraint") || value === "drop") { + } else if (value === "drop constraint" || value === "drop") { const result = this.parseDropConstraintAction(lexemes, idx); actions.push(result.value); idx = result.newIndex;
446-465: Deduplicate joinLexemeValues utility across parsers.This helper is duplicated here and in CreateTableParser. Extract to a shared utility to reduce drift and improve consistency.
packages/core/src/parsers/SqlPrintTokenParser.ts (1)
1814-1828: Remove noop alias check in visitTableSource.Alias handling is done in visitSourceExpression; this empty block is confusing.
- // alias (if present and different from table name) - if (arg.identifier && arg.identifier.name !== arg.table.name) { - - } + // Alias is handled by visitSourceExpression.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
docs/guide/formatting-recipes.md(1 hunks)packages/core/src/index.ts(2 hunks)packages/core/src/models/CreateTableQuery.ts(3 hunks)packages/core/src/models/DDLStatements.ts(1 hunks)packages/core/src/models/SqlPrintToken.ts(1 hunks)packages/core/src/parsers/AlterTableParser.ts(1 hunks)packages/core/src/parsers/CreateIndexParser.ts(1 hunks)packages/core/src/parsers/CreateTableParser.ts(3 hunks)packages/core/src/parsers/DropConstraintParser.ts(1 hunks)packages/core/src/parsers/DropIndexParser.ts(1 hunks)packages/core/src/parsers/DropTableParser.ts(1 hunks)packages/core/src/parsers/SqlParser.ts(4 hunks)packages/core/src/parsers/SqlPrintTokenParser.ts(4 hunks)packages/core/src/tokenReaders/CommandTokenReader.ts(1 hunks)packages/core/src/transformers/SqlPrinter.ts(1 hunks)packages/core/tests/parsers/CreateTableParser.test.ts(2 hunks)packages/core/tests/parsers/DDLParsers.test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
packages/core/**/*.{ts,tsx}
📄 CodeRabbit inference engine (packages/core/AGENTS.md)
packages/core/**/*.{ts,tsx}: Always compile and fix TypeScript errors before running tests
Source code comments must be written in English only
For model-driven mappings (mapping.typeInfo and mapping.structure), use convertModelDrivenMapping
Use imports from 'rawsql-ts' rather than local relative paths for library usage
Files:
packages/core/src/index.tspackages/core/src/parsers/DropTableParser.tspackages/core/src/parsers/DropIndexParser.tspackages/core/src/parsers/SqlParser.tspackages/core/tests/parsers/DDLParsers.test.tspackages/core/src/parsers/DropConstraintParser.tspackages/core/tests/parsers/CreateTableParser.test.tspackages/core/src/parsers/CreateIndexParser.tspackages/core/src/parsers/AlterTableParser.tspackages/core/src/parsers/CreateTableParser.tspackages/core/src/tokenReaders/CommandTokenReader.tspackages/core/src/models/CreateTableQuery.tspackages/core/src/models/DDLStatements.tspackages/core/src/parsers/SqlPrintTokenParser.tspackages/core/src/models/SqlPrintToken.tspackages/core/src/transformers/SqlPrinter.ts
packages/core/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (packages/core/AGENTS.md)
Remove console.log statements before commit
Files:
packages/core/src/index.tspackages/core/src/parsers/DropTableParser.tspackages/core/src/parsers/DropIndexParser.tspackages/core/src/parsers/SqlParser.tspackages/core/tests/parsers/DDLParsers.test.tspackages/core/src/parsers/DropConstraintParser.tspackages/core/tests/parsers/CreateTableParser.test.tspackages/core/src/parsers/CreateIndexParser.tspackages/core/src/parsers/AlterTableParser.tspackages/core/src/parsers/CreateTableParser.tspackages/core/src/tokenReaders/CommandTokenReader.tspackages/core/src/models/CreateTableQuery.tspackages/core/src/models/DDLStatements.tspackages/core/src/parsers/SqlPrintTokenParser.tspackages/core/src/models/SqlPrintToken.tspackages/core/src/transformers/SqlPrinter.ts
packages/core/**/*.{test,spec}.{ts,tsx,js}
📄 CodeRabbit inference engine (packages/core/AGENTS.md)
Tests are specs — never change expected values without user consultation
Files:
packages/core/tests/parsers/DDLParsers.test.tspackages/core/tests/parsers/CreateTableParser.test.ts
packages/core/**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (packages/core/AGENTS.md)
Use SqlFormatter for SQL comparison tests
Files:
packages/core/tests/parsers/DDLParsers.test.tspackages/core/tests/parsers/CreateTableParser.test.ts
🧬 Code graph analysis (12)
packages/core/src/parsers/DropTableParser.ts (5)
packages/core/src/models/DDLStatements.ts (2)
DropTableStatement(19-31)DropBehavior(12-12)packages/core/src/parsers/SqlTokenizer.ts (1)
SqlTokenizer(35-595)packages/core/src/models/Lexeme.ts (1)
Lexeme(42-48)packages/core/src/models/ValueComponent.ts (1)
QualifiedName(457-484)packages/core/src/parsers/FullNameParser.ts (1)
FullNameParser(9-106)
packages/core/src/parsers/DropIndexParser.ts (5)
packages/core/src/models/DDLStatements.ts (2)
DropIndexStatement(36-55)DropBehavior(12-12)packages/core/src/parsers/SqlTokenizer.ts (1)
SqlTokenizer(35-595)packages/core/src/models/Lexeme.ts (1)
Lexeme(42-48)packages/core/src/models/ValueComponent.ts (1)
QualifiedName(457-484)packages/core/src/parsers/FullNameParser.ts (1)
FullNameParser(9-106)
packages/core/src/parsers/SqlParser.ts (7)
packages/core/src/models/DDLStatements.ts (5)
DropTableStatement(19-31)DropIndexStatement(36-55)CreateIndexStatement(87-133)AlterTableStatement(182-201)DropConstraintStatement(206-218)packages/core/src/parsers/SqlTokenizer.ts (1)
StatementLexemeResult(23-30)packages/core/src/parsers/DropTableParser.ts (1)
DropTableParser(10-68)packages/core/src/parsers/DropIndexParser.ts (1)
DropIndexParser(10-75)packages/core/src/parsers/CreateIndexParser.ts (1)
CreateIndexParser(16-286)packages/core/src/parsers/AlterTableParser.ts (1)
AlterTableParser(26-466)packages/core/src/parsers/DropConstraintParser.ts (1)
DropConstraintParser(9-53)
packages/core/tests/parsers/DDLParsers.test.ts (6)
packages/core/src/parsers/DropTableParser.ts (1)
DropTableParser(10-68)packages/core/src/parsers/CreateIndexParser.ts (1)
CreateIndexParser(16-286)packages/core/src/models/DDLStatements.ts (4)
CreateIndexStatement(87-133)AlterTableStatement(182-201)AlterTableAddConstraint(138-154)AlterTableDropConstraint(159-175)packages/core/src/parsers/DropIndexParser.ts (1)
DropIndexParser(10-75)packages/core/src/parsers/AlterTableParser.ts (1)
AlterTableParser(26-466)packages/core/src/parsers/DropConstraintParser.ts (1)
DropConstraintParser(9-53)
packages/core/src/parsers/DropConstraintParser.ts (4)
packages/core/src/models/DDLStatements.ts (2)
DropConstraintStatement(206-218)DropBehavior(12-12)packages/core/src/parsers/SqlTokenizer.ts (1)
SqlTokenizer(35-595)packages/core/src/models/Lexeme.ts (1)
Lexeme(42-48)packages/core/src/parsers/FullNameParser.ts (1)
FullNameParser(9-106)
packages/core/tests/parsers/CreateTableParser.test.ts (1)
packages/core/src/parsers/CreateTableParser.ts (1)
CreateTableParser(47-730)
packages/core/src/parsers/CreateIndexParser.ts (5)
packages/core/src/models/DDLStatements.ts (4)
CreateIndexStatement(87-133)IndexColumnDefinition(60-82)IndexSortOrder(13-13)IndexNullsOrder(14-14)packages/core/src/models/Lexeme.ts (1)
Lexeme(42-48)packages/core/src/parsers/FullNameParser.ts (1)
FullNameParser(9-106)packages/core/src/models/ValueComponent.ts (4)
QualifiedName(457-484)IdentifierString(277-284)RawString(268-275)ValueComponent(5-27)packages/core/src/parsers/ValueParser.ts (1)
ValueParser(16-579)
packages/core/src/parsers/AlterTableParser.ts (6)
packages/core/src/models/CreateTableQuery.ts (7)
MatchType(19-19)ReferentialAction(16-16)ConstraintDeferrability(17-17)ConstraintInitially(18-18)TableConstraintDefinition(101-132)TableConstraintKind(96-96)ReferenceDefinition(24-52)packages/core/src/models/DDLStatements.ts (5)
AlterTableStatement(182-201)AlterTableAction(177-177)AlterTableAddConstraint(138-154)AlterTableDropConstraint(159-175)DropBehavior(12-12)packages/core/src/parsers/SqlTokenizer.ts (1)
SqlTokenizer(35-595)packages/core/src/models/Lexeme.ts (1)
Lexeme(42-48)packages/core/src/parsers/FullNameParser.ts (1)
FullNameParser(9-106)packages/core/src/models/ValueComponent.ts (4)
QualifiedName(457-484)IdentifierString(277-284)RawString(268-275)ValueComponent(5-27)
packages/core/src/parsers/CreateTableParser.ts (6)
packages/core/src/models/ValueComponent.ts (5)
IdentifierString(277-284)RawString(268-275)TypeValue(365-394)QualifiedName(457-484)ValueComponent(5-27)packages/core/src/models/CreateTableQuery.ts (10)
TableColumnDefinition(137-153)TableConstraintDefinition(101-132)ReferenceDefinition(24-52)MatchType(19-19)ReferentialAction(16-16)ConstraintDeferrability(17-17)ConstraintInitially(18-18)CreateTableQuery(156-248)ColumnConstraintDefinition(69-94)ColumnConstraintKind(54-64)packages/core/src/models/Lexeme.ts (1)
Lexeme(42-48)packages/core/src/parsers/FullNameParser.ts (1)
FullNameParser(9-106)packages/core/src/parsers/FunctionExpressionParser.ts (1)
FunctionExpressionParser(12-539)packages/core/src/parsers/ValueParser.ts (1)
ValueParser(16-579)
packages/core/src/models/CreateTableQuery.ts (3)
packages/core/src/models/ValueComponent.ts (7)
QualifiedName(457-484)IdentifierString(277-284)ValueComponent(5-27)RawString(268-275)TypeValue(365-394)ColumnReference(47-82)FunctionCall(84-123)packages/core/src/transformers/SelectValueCollector.ts (1)
SelectValueCollector(13-269)packages/core/src/models/Clause.ts (5)
SelectItem(6-15)SelectClause(17-28)FromClause(306-336)SourceExpression(242-260)TableSource(147-189)
packages/core/src/models/DDLStatements.ts (2)
packages/core/src/models/ValueComponent.ts (4)
QualifiedName(457-484)ValueComponent(5-27)IdentifierString(277-284)RawString(268-275)packages/core/src/models/CreateTableQuery.ts (1)
TableConstraintDefinition(101-132)
packages/core/src/parsers/SqlPrintTokenParser.ts (4)
packages/core/src/models/CreateTableQuery.ts (4)
TableColumnDefinition(137-153)ColumnConstraintDefinition(69-94)TableConstraintDefinition(101-132)ReferenceDefinition(24-52)packages/core/src/models/DDLStatements.ts (8)
CreateIndexStatement(87-133)IndexColumnDefinition(60-82)DropTableStatement(19-31)DropIndexStatement(36-55)AlterTableStatement(182-201)AlterTableAddConstraint(138-154)AlterTableDropConstraint(159-175)DropConstraintStatement(206-218)packages/core/src/models/SqlPrintToken.ts (1)
SqlPrintToken(110-140)packages/core/src/models/ValueComponent.ts (2)
QualifiedName(457-484)IdentifierString(277-284)
🔇 Additional comments (15)
packages/core/src/parsers/SqlParser.ts (8)
8-14: New DDL types imported — LGTM
23-28: New DDL parsers imported — LGTM
29-41: ParsedStatement union extended — LGTMType surface now includes DDL statements.
283-299: parseDropTableStatement — LGTMPlease add a smoke test covering: single table, multi-table, IF EXISTS, CASCADE/RESTRICT.
300-316: parseDropIndexStatement — LGTMAdd tests for CONCURRENTLY+IF EXISTS combos and multiple indexes.
317-333: parseCreateIndexStatement wiring — LGTM
334-350: parseAlterTableStatement wiring — LGTM
351-367: parseDropConstraintStatement wiring — LGTMpackages/core/src/index.ts (1)
11-15: Public exports for DDL parsers verified—no issues foundVerification confirms all parser and model exports are present and exported exactly once. Consider updating API documentation to reflect these new public symbols.
packages/core/src/tokenReaders/CommandTokenReader.ts (1)
111-111: Keywords added are correct; longest-match logic confirmedThe trie's design ensures "IF NOT EXISTS" is matched before "IF EXISTS"—when both are in the trie,
pushLexeme()returnsPartialOrFinalon "if", allowing the parser to consume "not exists" before considering a match complete. Test coverage in DDLParsers.test.ts validates both forms across DROP TABLE, CREATE INDEX, ALTER TABLE, and DROP CONSTRAINT. Parser code confirms lowercase matching and complete phrase checking. Identifier parsing safety is assured—these are SQL-reserved keywords and existing tests show no regressions.packages/core/src/transformers/SqlPrinter.ts (1)
168-171: Verify DDL indentation coverage is completeThe change adds indentation to 4 container types (SimpleSelectQuery, CreateTableDefinition, AlterTableStatement, IndexColumnList), but the enum defines 15 DDL-related containers. Other DDL types exist that may deserve consistent indentation:
- CreateIndexStatement, CreateTableQuery, TableConstraintDefinition, ColumnConstraintDefinition, TableColumnDefinition, IndexColumnDefinition, AlterTableAddConstraint, AlterTableDropConstraint
Confirm whether excluding these is intentional or if they should also be added to indentIncrementContainers for consistent DDL formatting.
packages/core/src/parsers/DropConstraintParser.ts (1)
20-52: LGTM — consistent DROP CONSTRAINT parsing.Token normalization, IF EXISTS, and behavior handling look correct; matches the printing expectations in tests.
packages/core/src/models/DDLStatements.ts (1)
101-133: LGTM on CreateIndexStatement normalization.Field defaults and deep copies follow existing patterns; aligns with parser/printer expectations.
packages/core/src/models/SqlPrintToken.ts (1)
86-100: Token containers aligned with printer handlers.New DDL container kinds match visit* methods; no issues spotted.
packages/core/src/parsers/CreateIndexParser.ts (1)
30-35: No change needed — tokenizer supports multi-word keywords.KeywordParser.parse concatenates matched identifiers into single lexeme values (e.g., "create index" / "create unique index") and CommandTokenReader registers those patterns, so CreateIndexParser's current checks are valid.
Likely an incorrect or invalid review comment.
| // Build a simple SELECT ... FROM table query. | ||
| const qualifiedName = this.namespaces && this.namespaces.length > 0 | ||
| ? [...this.namespaces, this.tableName.name].join(".") | ||
| : this.tableName.name; | ||
|
|
There was a problem hiding this comment.
Schema qualification bug: passing dotted string as table name breaks quoting.
You build "schema.table" as a single IdentifierString with namespaces=null, yielding wrong output like "schema.table". Pass namespaces and table separately to TableSource.
- // Build a simple SELECT ... FROM table query.
- const qualifiedName = this.namespaces && this.namespaces.length > 0
- ? [...this.namespaces, this.tableName.name].join(".")
- : this.tableName.name;
+ // Build a simple SELECT ... FROM table query.
+ const tableSource = new TableSource(this.namespaces ?? null, this.tableName);
@@
- new TableSource(null, qualifiedName),
+ tableSource,Also applies to: 219-223
🤖 Prompt for AI Agents
In packages/core/src/models/CreateTableQuery.ts around lines 210-214 (and
similarly 219-223), the code builds a dotted "schema.table" string and sets
namespaces=null which breaks identifier quoting; instead construct the
TableSource (or relevant AST node) with namespaces as an array and tableName as
a bare IdentifierString (no dots) so the renderer can quote each part
correctly—remove the concatenated qualifiedName and pass this.namespaces (if
any) and this.tableName.name separately when creating the TableSource/Identifier
node.
| const qualifiedName = this.namespaces && this.namespaces.length > 0 | ||
| ? [...this.namespaces, this.tableName.name].join(".") | ||
| : this.tableName.name; | ||
|
|
There was a problem hiding this comment.
Same issue in getCountQuery: use namespaces-aware TableSource.
Ensure proper qualification and quoting.
- const qualifiedName = this.namespaces && this.namespaces.length > 0
- ? [...this.namespaces, this.tableName.name].join(".")
- : this.tableName.name;
+ const tableSource = new TableSource(this.namespaces ?? null, this.tableName);
@@
- new TableSource(null, qualifiedName),
+ tableSource,Also applies to: 241-245
🤖 Prompt for AI Agents
packages/core/src/models/CreateTableQuery.ts lines 231-234 and 241-245: the code
builds a qualifiedName string manually and thus ignores the project's
TableSource logic for namespace-aware qualification and proper identifier
quoting; replace the manual qualifiedName construction with the project’s
TableSource helper (or factory) that accepts the TableName and namespaces so it
emits a correctly-qualified, quoted table reference (use the same TableSource
usage pattern as other queries in the repo), and update both locations to render
the TableSource instead of joining namespaces by hand.
| switch (arg.kind) { | ||
| case 'primary-key': | ||
| appendKeyword('primary key'); | ||
| appendColumns(arg.columns ?? []); | ||
| break; | ||
| case 'unique': | ||
| appendKeyword('unique'); | ||
| appendColumns(arg.columns ?? []); | ||
| break; | ||
| case 'foreign-key': | ||
| appendKeyword('foreign key'); | ||
| appendColumns(arg.columns ?? []); | ||
| if (arg.reference) { | ||
| token.innerTokens.push(SqlPrintTokenParser.SPACE_TOKEN); | ||
| token.innerTokens.push(arg.reference.accept(this)); | ||
| } | ||
| break; | ||
| case 'check': | ||
| if (arg.checkExpression) { | ||
| appendKeyword('check'); | ||
| token.innerTokens.push(SqlPrintTokenParser.SPACE_TOKEN); | ||
| token.innerTokens.push(SqlPrintTokenParser.PAREN_OPEN_TOKEN); | ||
| token.innerTokens.push(this.visit(arg.checkExpression)); | ||
| token.innerTokens.push(SqlPrintTokenParser.PAREN_CLOSE_TOKEN); | ||
| } | ||
| break; | ||
| case 'raw': | ||
| if (arg.rawClause) { | ||
| appendComponent(arg.rawClause); | ||
| } | ||
| break; | ||
| } | ||
|
|
||
| return token; | ||
| } |
There was a problem hiding this comment.
Table constraints drop deferrable/initially options.
Model exposes deferrable/initially, but printer omits them. Append when present to avoid data loss.
@@
switch (arg.kind) {
@@
}
+ // Constraint-level deferrability and initially options
+ if (arg.deferrable === 'deferrable') {
+ token.innerTokens.push(SqlPrintTokenParser.SPACE_TOKEN);
+ token.innerTokens.push(new SqlPrintToken(SqlPrintTokenType.keyword, 'deferrable'));
+ } else if (arg.deferrable === 'not deferrable') {
+ token.innerTokens.push(SqlPrintTokenParser.SPACE_TOKEN);
+ token.innerTokens.push(new SqlPrintToken(SqlPrintTokenType.keyword, 'not deferrable'));
+ }
+ if (arg.initially === 'immediate') {
+ token.innerTokens.push(SqlPrintTokenParser.SPACE_TOKEN);
+ token.innerTokens.push(new SqlPrintToken(SqlPrintTokenType.keyword, 'initially immediate'));
+ } else if (arg.initially === 'deferred') {
+ token.innerTokens.push(SqlPrintTokenParser.SPACE_TOKEN);
+ token.innerTokens.push(new SqlPrintToken(SqlPrintTokenType.keyword, 'initially deferred'));
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| switch (arg.kind) { | |
| case 'primary-key': | |
| appendKeyword('primary key'); | |
| appendColumns(arg.columns ?? []); | |
| break; | |
| case 'unique': | |
| appendKeyword('unique'); | |
| appendColumns(arg.columns ?? []); | |
| break; | |
| case 'foreign-key': | |
| appendKeyword('foreign key'); | |
| appendColumns(arg.columns ?? []); | |
| if (arg.reference) { | |
| token.innerTokens.push(SqlPrintTokenParser.SPACE_TOKEN); | |
| token.innerTokens.push(arg.reference.accept(this)); | |
| } | |
| break; | |
| case 'check': | |
| if (arg.checkExpression) { | |
| appendKeyword('check'); | |
| token.innerTokens.push(SqlPrintTokenParser.SPACE_TOKEN); | |
| token.innerTokens.push(SqlPrintTokenParser.PAREN_OPEN_TOKEN); | |
| token.innerTokens.push(this.visit(arg.checkExpression)); | |
| token.innerTokens.push(SqlPrintTokenParser.PAREN_CLOSE_TOKEN); | |
| } | |
| break; | |
| case 'raw': | |
| if (arg.rawClause) { | |
| appendComponent(arg.rawClause); | |
| } | |
| break; | |
| } | |
| return token; | |
| } | |
| switch (arg.kind) { | |
| case 'primary-key': | |
| appendKeyword('primary key'); | |
| appendColumns(arg.columns ?? []); | |
| break; | |
| case 'unique': | |
| appendKeyword('unique'); | |
| appendColumns(arg.columns ?? []); | |
| break; | |
| case 'foreign-key': | |
| appendKeyword('foreign key'); | |
| appendColumns(arg.columns ?? []); | |
| if (arg.reference) { | |
| token.innerTokens.push(SqlPrintTokenParser.SPACE_TOKEN); | |
| token.innerTokens.push(arg.reference.accept(this)); | |
| } | |
| break; | |
| case 'check': | |
| if (arg.checkExpression) { | |
| appendKeyword('check'); | |
| token.innerTokens.push(SqlPrintTokenParser.SPACE_TOKEN); | |
| token.innerTokens.push(SqlPrintTokenParser.PAREN_OPEN_TOKEN); | |
| token.innerTokens.push(this.visit(arg.checkExpression)); | |
| token.innerTokens.push(SqlPrintTokenParser.PAREN_CLOSE_TOKEN); | |
| } | |
| break; | |
| case 'raw': | |
| if (arg.rawClause) { | |
| appendComponent(arg.rawClause); | |
| } | |
| break; | |
| } | |
| // Constraint-level deferrability and initially options | |
| if (arg.deferrable === 'deferrable') { | |
| token.innerTokens.push(SqlPrintTokenParser.SPACE_TOKEN); | |
| token.innerTokens.push(new SqlPrintToken(SqlPrintTokenType.keyword, 'deferrable')); | |
| } else if (arg.deferrable === 'not deferrable') { | |
| token.innerTokens.push(SqlPrintTokenParser.SPACE_TOKEN); | |
| token.innerTokens.push(new SqlPrintToken(SqlPrintTokenType.keyword, 'not deferrable')); | |
| } | |
| if (arg.initially === 'immediate') { | |
| token.innerTokens.push(SqlPrintTokenParser.SPACE_TOKEN); | |
| token.innerTokens.push(new SqlPrintToken(SqlPrintTokenType.keyword, 'initially immediate')); | |
| } else if (arg.initially === 'deferred') { | |
| token.innerTokens.push(SqlPrintTokenParser.SPACE_TOKEN); | |
| token.innerTokens.push(new SqlPrintToken(SqlPrintTokenType.keyword, 'initially deferred')); | |
| } | |
| return token; | |
| } |
…rs with joinLexemeValues utility
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
packages/core/src/parsers/CreateIndexParser.ts (3)
228-258: Unterminated WITH options aren't detected.When the closing
)is missing, the loop exits at EOF without error. Add validation for balanced parentheses.Apply:
idx++; } +if (depth !== 0) { + throw new Error(`[CreateIndexParser] Unterminated WITH options starting at index ${start}; unbalanced parentheses.`); +} + const text = joinLexemeValues(lexemes, start, idx);
201-226: Missing error on unclosed identifier list (INCLUDE).Similar EOF behavior as the column list. Add validation to throw if
)isn't found.Apply this diff:
} + if (idx >= lexemes.length || lexemes[idx - 1]?.type !== TokenType.CloseParen) { + throw new Error(`[CreateIndexParser] Expected ')' to close identifier list starting at index ${index}.`); + } + return { identifiers, newIndex: idx }; }
124-199: Missing error on unclosed column list.The prior review correctly identified this issue. When EOF occurs before
), the function returns without error. Add validation to throw when the list isn't properly closed.Apply this diff:
} + if (idx >= lexemes.length || lexemes[idx - 1]?.type !== TokenType.CloseParen) { + throw new Error(`[CreateIndexParser] Expected ')' to close column list starting at index ${index}.`); + } + return { columns, newIndex: idx }; }
🧹 Nitpick comments (1)
packages/core/src/parsers/AlterTableParser.ts (1)
303-329: Consider validating closed identifier lists.Similar to CreateTableParser and CreateIndexParser, this loop exits silently at EOF without ensuring
)was encountered. While this is consistent across parsers, adding explicit validation would improve error messages for malformed SQL.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
docs/guide/formatting-recipes.md(1 hunks)packages/core/src/parsers/AlterTableParser.ts(1 hunks)packages/core/src/parsers/CreateIndexParser.ts(1 hunks)packages/core/src/parsers/CreateTableParser.ts(2 hunks)packages/core/src/parsers/DropIndexParser.ts(1 hunks)packages/core/src/utils/ParserStringUtils.ts(1 hunks)packages/core/tests/parsers/DDLParsers.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/core/tests/parsers/DDLParsers.test.ts
🧰 Additional context used
📓 Path-based instructions (2)
packages/core/**/*.{ts,tsx}
📄 CodeRabbit inference engine (packages/core/AGENTS.md)
packages/core/**/*.{ts,tsx}: Always compile and fix TypeScript errors before running tests
Source code comments must be written in English only
For model-driven mappings (mapping.typeInfo and mapping.structure), use convertModelDrivenMapping
Use imports from 'rawsql-ts' rather than local relative paths for library usage
Files:
packages/core/src/utils/ParserStringUtils.tspackages/core/src/parsers/CreateTableParser.tspackages/core/src/parsers/DropIndexParser.tspackages/core/src/parsers/AlterTableParser.tspackages/core/src/parsers/CreateIndexParser.ts
packages/core/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (packages/core/AGENTS.md)
Remove console.log statements before commit
Files:
packages/core/src/utils/ParserStringUtils.tspackages/core/src/parsers/CreateTableParser.tspackages/core/src/parsers/DropIndexParser.tspackages/core/src/parsers/AlterTableParser.tspackages/core/src/parsers/CreateIndexParser.ts
🧬 Code graph analysis (5)
packages/core/src/utils/ParserStringUtils.ts (1)
packages/core/src/models/Lexeme.ts (1)
Lexeme(42-48)
packages/core/src/parsers/CreateTableParser.ts (7)
packages/core/src/models/ValueComponent.ts (5)
IdentifierString(277-284)RawString(268-275)TypeValue(365-394)QualifiedName(457-484)ValueComponent(5-27)packages/core/src/models/CreateTableQuery.ts (10)
TableColumnDefinition(137-153)TableConstraintDefinition(101-132)ReferenceDefinition(24-52)MatchType(19-19)ReferentialAction(16-16)ConstraintDeferrability(17-17)ConstraintInitially(18-18)CreateTableQuery(156-248)ColumnConstraintDefinition(69-94)ColumnConstraintKind(54-64)packages/core/src/models/Lexeme.ts (1)
Lexeme(42-48)packages/core/src/utils/ParserStringUtils.ts (1)
joinLexemeValues(10-33)packages/core/src/parsers/FullNameParser.ts (1)
FullNameParser(9-106)packages/core/src/parsers/FunctionExpressionParser.ts (1)
FunctionExpressionParser(12-539)packages/core/src/parsers/ValueParser.ts (1)
ValueParser(16-579)
packages/core/src/parsers/DropIndexParser.ts (5)
packages/core/src/models/DDLStatements.ts (2)
DropIndexStatement(36-55)DropBehavior(12-12)packages/core/src/parsers/SqlTokenizer.ts (1)
SqlTokenizer(35-595)packages/core/src/models/Lexeme.ts (1)
Lexeme(42-48)packages/core/src/models/ValueComponent.ts (1)
QualifiedName(457-484)packages/core/src/parsers/FullNameParser.ts (1)
FullNameParser(9-106)
packages/core/src/parsers/AlterTableParser.ts (7)
packages/core/src/models/CreateTableQuery.ts (7)
MatchType(19-19)ReferentialAction(16-16)ConstraintDeferrability(17-17)ConstraintInitially(18-18)TableConstraintDefinition(101-132)TableConstraintKind(96-96)ReferenceDefinition(24-52)packages/core/src/models/DDLStatements.ts (4)
AlterTableAction(177-177)AlterTableAddConstraint(138-154)AlterTableDropConstraint(159-175)DropBehavior(12-12)packages/core/src/parsers/SqlTokenizer.ts (1)
SqlTokenizer(35-595)packages/core/src/parsers/FullNameParser.ts (1)
FullNameParser(9-106)packages/core/src/models/ValueComponent.ts (4)
QualifiedName(457-484)IdentifierString(277-284)RawString(268-275)ValueComponent(5-27)packages/core/src/utils/ParserStringUtils.ts (1)
joinLexemeValues(10-33)packages/core/src/parsers/ValueParser.ts (1)
ValueParser(16-579)
packages/core/src/parsers/CreateIndexParser.ts (6)
packages/core/src/models/DDLStatements.ts (4)
CreateIndexStatement(87-133)IndexColumnDefinition(60-82)IndexSortOrder(13-13)IndexNullsOrder(14-14)packages/core/src/parsers/SqlTokenizer.ts (1)
SqlTokenizer(35-595)packages/core/src/parsers/FullNameParser.ts (1)
FullNameParser(9-106)packages/core/src/models/ValueComponent.ts (4)
QualifiedName(457-484)IdentifierString(277-284)RawString(268-275)ValueComponent(5-27)packages/core/src/parsers/ValueParser.ts (1)
ValueParser(16-579)packages/core/src/utils/ParserStringUtils.ts (1)
joinLexemeValues(10-33)
🔇 Additional comments (24)
packages/core/src/parsers/CreateTableParser.ts (13)
3-22: LGTM!The imports are comprehensive and correctly organized, bringing in all necessary models, parsers, and utilities for DDL parsing.
24-43: LGTM!The internal result interfaces are well-typed and provide clear contracts for parsing helpers.
49-96: LGTM!The static constants provide centralized keyword recognition, improving maintainability and reducing magic strings throughout the parser.
101-109: LGTM!The parse method follows the standard pattern with proper tokenization and trailing token validation.
114-206: LGTM!The main parsing logic is thorough and handles multiple DDL forms (with/without column definitions, with/without AS SELECT). The comment preservation ensures metadata isn't lost during parsing.
208-215: LGTM!Proper delegation with defensive array copying.
264-299: LGTM!The column definition parsing correctly rejects schema-qualified column names and handles optional data types with constraint collection.
301-310: LGTM!The type parsing with fallback to raw string ensures robust handling of both structured and unrecognized type syntax.
312-439: LGTM!The column constraint parsing covers a wide range of SQL constraint types with a sensible fallback for unsupported syntax.
441-532: LGTM!Table constraint parsing mirrors the column constraint structure and correctly propagates deferrability options from reference definitions.
562-649: LGTM!The REFERENCES clause parsing comprehensively handles all standard SQL foreign key options with proper validation through keyword maps.
651-664: LGTM!The parenthesized expression parser properly validates both opening and closing parentheses.
666-713: LGTM!The utility predicates handle edge cases well, particularly the WITH keyword disambiguation between CTE and table options.
packages/core/src/utils/ParserStringUtils.ts (1)
1-32: LGTM!The
joinLexemeValuesutility provides clean whitespace normalization for reconstructing SQL fragments, correctly handling punctuation and dots.docs/guide/formatting-recipes.md (1)
180-213: LGTM!The documentation clearly explains DDL formatting capabilities with a practical example and references to all relevant parsers.
packages/core/src/parsers/DropIndexParser.ts (1)
1-79: LGTM!The DROP INDEX parser is clean and correctly validates modifiers, index names, and optional behavior clauses. The validation after IF EXISTS (lines 42-44) is particularly good.
packages/core/src/parsers/CreateIndexParser.ts (1)
18-122: LGTM!The main parsing flow in
parseandparseFromLexemeis well-structured with proper modifier handling, qualified name parsing, and clause parsing.packages/core/src/parsers/AlterTableParser.ts (7)
1-58: LGTM!The imports and static constants are well-organized and consistent with CreateTableParser's approach.
60-127: LGTM!The main parsing logic correctly handles IF EXISTS, ONLY modifier, and dispatches to action-specific parsers with proper validation.
129-174: LGTM!The ADD CONSTRAINT action parser handles optional CONSTRAINT keyword, IF NOT EXISTS, constraint name, and NOT VALID modifier correctly.
176-216: LGTM!The DROP CONSTRAINT action parser correctly handles both composite and split DROP CONSTRAINT tokens with proper validation.
218-301: LGTM!The constraint definition parsing covers all standard constraint types with consistent fallback to raw clause capture.
331-415: LGTM!The REFERENCES definition parsing is comprehensive and matches CreateTableParser's implementation, maintaining consistency.
417-445: LGTM!The parenthesized expression parser and clause end finder provide clean helper functionality with proper validation.
| private static parseDefinitionList( | ||
| lexemes: Lexeme[], | ||
| index: number | ||
| ): { columns: TableColumnDefinition[]; tableConstraints: TableConstraintDefinition[]; newIndex: number } { | ||
| let idx = index; | ||
| const columns: TableColumnDefinition[] = []; | ||
| const constraints: TableConstraintDefinition[] = []; | ||
|
|
||
| // Skip opening parenthesis. | ||
| idx++; | ||
|
|
||
| // Parse individual column or constraint entries until closing parenthesis. | ||
| while (idx < lexemes.length) { | ||
| const lexeme = lexemes[idx]; | ||
| if (lexeme.type === TokenType.CloseParen) { | ||
| idx++; | ||
| break; | ||
| } | ||
|
|
||
| const tokenValue = lexeme.value.toLowerCase(); | ||
|
|
||
| if (this.TABLE_CONSTRAINT_STARTERS.has(tokenValue)) { | ||
| const constraintResult = this.parseTableConstraint(lexemes, idx); | ||
| constraints.push(constraintResult.value); | ||
| idx = constraintResult.newIndex; | ||
| } else { | ||
| const columnResult = this.parseColumnDefinition(lexemes, idx); | ||
| columns.push(columnResult.value); | ||
| idx = columnResult.newIndex; | ||
| } | ||
|
|
||
| // Consume delimiter comma between definitions. | ||
| if (idx < lexemes.length && (lexemes[idx].type & TokenType.Comma)) { | ||
| idx++; | ||
| continue; | ||
| } | ||
|
|
||
| // Break when encountering the closing parenthesis. | ||
| if (idx < lexemes.length && lexemes[idx].type === TokenType.CloseParen) { | ||
| idx++; | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| return { columns, tableConstraints: constraints, newIndex: idx }; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Check for unclosed definition list.
If the definition list reaches EOF without a closing ), the loop exits silently and returns. Consider adding validation to throw an error when the list isn't properly terminated.
🏁 Script executed:
#!/bin/bash
# Check if other parsers in the same directory have similar parenthesis validation patterns
rg -nP -A3 'OpenParen.*\n.*while.*idx.*<.*length' packages/core/src/parsers/Length of output: 0
🏁 Script executed:
#!/bin/bash
# List parser files and search for error handling patterns
echo "=== Parser files ==="
fd -t f '\.ts$' packages/core/src/parsers/ | head -20
echo -e "\n=== Search for throw statements in parsers ==="
rg -n 'throw ' packages/core/src/parsers/ -A1 | head -30
echo -e "\n=== Look for how EOF/boundary conditions are handled ==="
rg -n 'idx >= lexemes\.length' packages/core/src/parsers/ -B2 -A2 | head -40Length of output: 6978
🏁 Script executed:
#!/bin/bash
# Look at parseTableConstraint and parseColumnDefinition to understand expected behavior
echo "=== parseTableConstraint implementation ==="
ast-grep --pattern 'parseTableConstraint($_, $_) { $$$ }'
echo -e "\n=== parseColumnDefinition implementation ==="
ast-grep --pattern 'parseColumnDefinition($_, $_) { $$$ }'Length of output: 89
🏁 Script executed:
#!/bin/bash
# Search for how the parseDefinitionList result is used (check caller expectations)
rg -n 'parseDefinitionList' packages/core/src/parsers/ -B3 -A3Length of output: 1241
Add validation to throw an error if the definition list reaches EOF without closing parenthesis.
The method exits silently when EOF is reached without a closing ), which is inconsistent with error-handling patterns used throughout the codebase (e.g., WindowExpressionParser line 52-54, WindowClauseParser line 27-29). Add a check after the loop: if idx >= lexemes.length without encountering CloseParen, throw an error with a descriptive message indicating the unclosed definition list.
🤖 Prompt for AI Agents
In packages/core/src/parsers/CreateTableParser.ts around lines 217-262, the
parseDefinitionList loop can reach EOF without finding a CloseParen and
currently returns silently; add a post-loop validation that if idx >=
lexemes.length (i.e. EOF reached) you throw an error indicating the definition
list was not closed (e.g. "Unterminated definition list: expected ')' before end
of input" and include the current index/lexeme context), following the same
error-handling pattern used in the other parsers so callers get a clear failure
rather than silent success.
| private static parseIdentifierList(lexemes: Lexeme[], index: number): { identifiers: IdentifierString[]; newIndex: number } { | ||
| let idx = index; | ||
| const identifiers: IdentifierString[] = []; | ||
|
|
||
| if (lexemes[idx]?.type !== TokenType.OpenParen) { | ||
| throw new Error(`[CreateTableParser] Expected '(' to start identifier list at index ${idx}.`); | ||
| } | ||
| idx++; | ||
|
|
||
| while (idx < lexemes.length) { | ||
| const nameResult = this.parseQualifiedName(lexemes, idx); | ||
| identifiers.push(nameResult.name); | ||
| idx = nameResult.newIndex; | ||
|
|
||
| if (lexemes[idx]?.type === TokenType.Comma) { | ||
| idx++; | ||
| continue; | ||
| } | ||
|
|
||
| if (lexemes[idx]?.type === TokenType.CloseParen) { | ||
| idx++; | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| return { identifiers, newIndex: idx }; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Check for unclosed identifier list.
Similar to the definition list, this loop exits silently at EOF without ensuring the closing ) was encountered. Consider adding validation.
🏁 Script executed:
#!/bin/bash
# Check how AlterTableParser handles the same pattern
rg -nP -A5 'parseIdentifierList' packages/core/src/parsers/AlterTableParser.tsLength of output: 1558
[/run_scripts]
🏁 Script executed:
#!/bin/bash
# Get the full parseIdentifierList implementation from CreateTableParser
sed -n '534,560p' packages/core/src/parsers/CreateTableParser.tsLength of output: 903
🏁 Script executed:
#!/bin/bash
# Get the full parseIdentifierList implementation from AlterTableParser
rg -nP -A30 'private static parseIdentifierList' packages/core/src/parsers/AlterTableParser.ts | head -40Length of output: 1259
[/run_scripts]
🏁 Script executed:
#!/bin/bash
# Check parseDefinitionList in CreateTableParser to compare patterns
rg -nP -A35 'private static parseDefinitionList' packages/core/src/parsers/CreateTableParser.ts | head -45Length of output: 1546
[/run_scripts]
🏁 Script executed:
#!/bin/bash
# Get full parseDefinitionList implementation
rg -nP -A50 'private static parseDefinitionList' packages/core/src/parsers/CreateTableParser.ts | head -65Length of output: 2091
Add validation for closing parenthesis in parseIdentifierList.
The method exits the loop at EOF without ensuring the closing ) was encountered. If input is malformed (e.g., missing closing paren), the method silently returns partially parsed data. Add validation after the loop, similar to checking the opening paren: if (lexemes[idx - 1]?.type !== TokenType.CloseParen) throw new Error(...).
This same issue exists in parseDefinitionList (lines 217–262) and should be fixed there as well.
🤖 Prompt for AI Agents
In packages/core/src/parsers/CreateTableParser.ts around lines 534 to 560,
parseIdentifierList advances until EOF without asserting a closing ')' so a
malformed input can return partial results; after the loop validate that the
last consumed token is a CloseParen and throw a clear Error if not (e.g.,
"Expected ')' to close identifier list at index X"). Apply the same fix to
parseDefinitionList (around lines 217–262): after its parsing loop assert a
closing ')' and throw an error when missing to prevent silently returning
incomplete parse state.
…r and refine index name validation in DropIndexParser
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
packages/core/src/parsers/DropIndexParser.ts (2)
55-57: Use bitmask check for punctuation tokensElsewhere you use bitwise checks (e.g., Dot). Comma should follow the same pattern to avoid strict-equality pitfalls.
- if (lexemes[idx]?.type === TokenType.Comma) { + if (lexemes[idx]?.type & TokenType.Comma) { idx++; continue; }
47-61: Optional: enforce PG rule for CONCURRENTLY with a single indexPostgreSQL forbids dropping multiple indexes with CONCURRENTLY. Consider validating:
const indexNames: QualifiedName[] = []; @@ while (idx < lexemes.length) { const { namespaces, name, newIndex } = FullNameParser.parseFromLexeme(lexemes, idx); indexNames.push(new QualifiedName(namespaces, name)); idx = newIndex; @@ break; } + + // Optional dialect rule (PostgreSQL): + // if (concurrently && indexNames.length > 1) { + // throw new Error("[DropIndexParser] CONCURRENTLY cannot be used with multiple index names."); + // }If cross-dialect support is desired, gate this behind a dialect option.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/core/src/parsers/CreateIndexParser.ts(1 hunks)packages/core/src/parsers/DropIndexParser.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/core/src/parsers/CreateIndexParser.ts
🧰 Additional context used
📓 Path-based instructions (2)
packages/core/**/*.{ts,tsx}
📄 CodeRabbit inference engine (packages/core/AGENTS.md)
packages/core/**/*.{ts,tsx}: Always compile and fix TypeScript errors before running tests
Source code comments must be written in English only
For model-driven mappings (mapping.typeInfo and mapping.structure), use convertModelDrivenMapping
Use imports from 'rawsql-ts' rather than local relative paths for library usage
Files:
packages/core/src/parsers/DropIndexParser.ts
packages/core/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (packages/core/AGENTS.md)
Remove console.log statements before commit
Files:
packages/core/src/parsers/DropIndexParser.ts
🧬 Code graph analysis (1)
packages/core/src/parsers/DropIndexParser.ts (4)
packages/core/src/models/DDLStatements.ts (2)
DropIndexStatement(36-55)DropBehavior(12-12)packages/core/src/parsers/SqlTokenizer.ts (1)
SqlTokenizer(35-595)packages/core/src/models/ValueComponent.ts (1)
QualifiedName(457-484)packages/core/src/parsers/FullNameParser.ts (1)
FullNameParser(9-106)
🔇 Additional comments (1)
packages/core/src/parsers/DropIndexParser.ts (1)
11-19: No issues found — semicolon handling is correctVerification confirms
SqlTokenizer.readLexemes()properly excludes trailing semicolons from the returned lexemes array. The tokenizer breaks statement parsing at;(line 176 of SqlTokenizer.ts) and explicitly skips past the terminator viaskipPastTerminator(), so valid inputs likeDROP INDEX idx;are parsed correctly without errors.
| if (lexemes[idx]?.value.toLowerCase() !== "drop index") { | ||
| throw new Error(`[DropIndexParser] Expected DROP INDEX at index ${idx}.`); | ||
| } | ||
| idx++; | ||
|
|
There was a problem hiding this comment.
Incorrect matching of multi-word keyword and null-safety bug
- "DROP INDEX" is matched as a single lexeme; most tokenizers emit "DROP" and "INDEX" separately.
lexemes[idx]?.value.toLowerCase()will throw iflexemes[idx]is undefined (missing?.beforetoLowerCase).
Fix by checking two tokens and making toLowerCase null-safe:
- if (lexemes[idx]?.value.toLowerCase() !== "drop index") {
- throw new Error(`[DropIndexParser] Expected DROP INDEX at index ${idx}.`);
- }
- idx++;
+ const kw = (i: number) => lexemes[i]?.value?.toLowerCase();
+ if (kw(idx) !== "drop" || kw(idx + 1) !== "index") {
+ throw new Error(`[DropIndexParser] Expected DROP INDEX at index ${idx}.`);
+ }
+ idx += 2;🤖 Prompt for AI Agents
In packages/core/src/parsers/DropIndexParser.ts around lines 24 to 28, the code
tries to match the multi-word keyword "DROP INDEX" as a single lexeme and calls
toLowerCase unsafely; update the check to verify two separate tokens: ensure
lexemes[idx] and lexemes[idx+1] exist, compare lexemes[idx].value and
lexemes[idx+1].value (using value?.toLowerCase()) to "drop" and "index"
respectively, throw the same error if either token is missing or mismatched, and
advance idx by 2 when the match succeeds (ensuring no undefined access to
toLowerCase).
| // Parse optional CONCURRENTLY modifier. | ||
| let concurrently = false; | ||
| if (lexemes[idx]?.value.toLowerCase() === "concurrently") { | ||
| concurrently = true; | ||
| idx++; | ||
| } | ||
|
|
||
| // Parse optional IF EXISTS modifier. | ||
| let ifExists = false; | ||
| if (lexemes[idx]?.value.toLowerCase() === "if exists") { | ||
| ifExists = true; | ||
| idx++; | ||
| const next = lexemes[idx]; | ||
| if (!next || next.type !== TokenType.Identifier) { | ||
| throw new Error("[DropIndexParser] Expected index name immediately after IF EXISTS."); | ||
| } | ||
| } |
There was a problem hiding this comment.
IF EXISTS parsed as one token and invalid identifier guard
- "IF EXISTS" is two tokens; current code treats it as one.
- The Identifier-only guard after IF EXISTS rejects valid bracketed/quoted names and fully qualified names handled by FullNameParser.
Simplify and make both clauses robust:
- // Parse optional CONCURRENTLY modifier.
- let concurrently = false;
- if (lexemes[idx]?.value.toLowerCase() === "concurrently") {
- concurrently = true;
- idx++;
- }
-
- // Parse optional IF EXISTS modifier.
- let ifExists = false;
- if (lexemes[idx]?.value.toLowerCase() === "if exists") {
- ifExists = true;
- idx++;
- const next = lexemes[idx];
- if (!next || next.type !== TokenType.Identifier) {
- throw new Error("[DropIndexParser] Expected index name immediately after IF EXISTS.");
- }
- }
+ // Parse optional CONCURRENTLY modifier.
+ let concurrently = false;
+ if (kw(idx) === "concurrently") {
+ concurrently = true;
+ idx++;
+ }
+
+ // Parse optional IF EXISTS modifier.
+ let ifExists = false;
+ if (kw(idx) === "if" && kw(idx + 1) === "exists") {
+ ifExists = true;
+ idx += 2;
+ }Committable suggestion skipped: line range outside the PR's diff.
| const nextValue = lexemes[idx]?.value.toLowerCase(); | ||
| if (nextValue === "cascade" || nextValue === "restrict") { | ||
| behavior = nextValue as DropBehavior; | ||
| idx++; | ||
| } |
There was a problem hiding this comment.
Null-safety for behavior token read
lexemes[idx]?.value.toLowerCase() can throw when idx === lexemes.length. Make the call null-safe.
- const nextValue = lexemes[idx]?.value.toLowerCase();
+ const nextValue = lexemes[idx]?.value?.toLowerCase();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const nextValue = lexemes[idx]?.value.toLowerCase(); | |
| if (nextValue === "cascade" || nextValue === "restrict") { | |
| behavior = nextValue as DropBehavior; | |
| idx++; | |
| } | |
| const nextValue = lexemes[idx]?.value?.toLowerCase(); | |
| if (nextValue === "cascade" || nextValue === "restrict") { | |
| behavior = nextValue as DropBehavior; | |
| idx++; | |
| } |
🤖 Prompt for AI Agents
In packages/core/src/parsers/DropIndexParser.ts around lines 68 to 72, the call
lexemes[idx]?.value.toLowerCase() can throw when idx === lexemes.length because
value may be undefined; make the access null-safe by either checking idx <
lexemes.length and lexemes[idx].value before calling toLowerCase or use optional
chaining on value (e.g., lexemes[idx]?.value?.toLowerCase()) and only compare
when nextValue is a non-empty string, so behavior is assigned only if nextValue
is defined and matches "cascade" or "restrict".
1 participant
Summary by CodeRabbit
New Features
Documentation
Tests