TylerBrinks / SqlParser-cs

A Friendly SQL Parser for .NET
MIT License
109 stars 20 forks source link

Bugfix: Autoincrement parsing #32

Closed julescmay closed 2 months ago

julescmay commented 2 months ago

I found a bug in the AUTOINCREMENT processing. Given something like this (derived from SQLite's chinook database):

CREATE TABLE "albums" ([AlbumId] INTEGER PRIMARY KEY AUTOINCREMENT NOT NULL) 

I got an error (using the SQLite dialect, which I needed bacause of the square brackets):

Expected ',' or ')' after column definition, reading NOT

The malfunction seems to be that, after failing to read AUTO_INCREMENT, it successfully reads AUTOINCREMENT, but then rejects it because it's neither MySql nor generic.

I think there are two problems. The first is the logic is wrong. What I think you intend (and what I've encoded into this) is: when _dialect is either MySql or generic, then accept AUTO_INCREMENT, otherwise accept AUTOINCREMENT. The second problem is that it should only try to parse it if there's a chance of accepting it (otherwise, it eats the word without recognising it, and creates a phase error), so I've moved the guard condition to before the parse attempt.

Overall, I'm unsure why you have the guard condition. Auto-increment syntax is so different between databases, I'm surprised there's a generic solution available at all.

julescmay commented 2 months ago

By the way, I found several other places where the guard condition followed the parse, e.g:

    if (ParseKeywordSequence(Keyword.ON, Keyword.UPDATE) && _dialect is MySqlDialect or GenericDialect)
    {
        return new ColumnOption.OnUpdate(ParseExpr());
    }

I suspect they all should be chnaged. Would you agree?

TylerBrinks commented 2 months ago

You found a legitimate typo. It should be a SQLite-specific dialect in the "AUTOINCREMENT" case. I've pushed a change to accommodate the fix.

The original implementation can be found here: https://github.com/sqlparser-rs/sqlparser-rs/blob/4d52ee728017248e2ec4cdc7a1a32eeb96619bdb/src/parser/mod.rs#L5994

TylerBrinks commented 2 months ago

You're correct, the dialect logic should precede the keyword parsing

TylerBrinks commented 2 months ago

Your change was merged along with the other dialect check reordering. Thanks!