sqldelight icon indicating copy to clipboard operation
sqldelight copied to clipboard

SQL statements in lowercase provided confusing error messages

Open staktrace opened this issue 2 years ago • 2 comments

SQLDelight Version

1.5.3

SQLDelight Dialect

mysql

Describe the Bug

I was migrating an existing project to use SQLDelight. I had the following in the first migration schema file:

create table foo (
`id` bigint primary key not null auto_increment,
`created_at` timestamp not null default current_timestamp on update current_timestamp,
`name` varchar(32) not null unique,
key `index_on_name` (`name`)
) engine=InnoDB default charset=utf8mb4;

and I got a ream of error messages, the first of which was this:

/path/to/migration.sqm line 1:7 - '{' expected, got 'table'
1    table
            ^^^^^

It took a couple of minutes to figure out why this was failing since the error message wasn't too helpful but the problem turned out to be case-sensitivity. Although the SQL statement as written works fine in a MySQL 5.7 database, it needs to be converted to uppercase for SQLDelight to recognize it:

CREATE TABLE foo (
`id` bigint PRIMARY KEY NOT NULL AUTO_INCREMENT,
`created_at` timestamp NOT NULL DEFAULT CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP,
`name` varchar(32) NOT NULL UNIQUE,
KEY `index_on_name` (`name`)
) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4;

It would be great if the SQL parsing engine in SQLDelight could detect this (probably frequent) error and emit a more useful error message. Or even better, just accept the lowercase SQL.

Stacktrace

No response

staktrace avatar Jun 28 '22 13:06 staktrace

I was also very surprised by this behavior. SQL as a language is case-insensitive with regard to keywords and should be supported as such.

I got this error for SQLite dialect, but I suspect it is the same for all dialects.

alllex avatar Jul 07 '22 06:07 alllex

its unfortunate because only treating uppercase as the keywords means you can do stuff like:

update:
UPDATE foo SET bar = ?;

and I like having sql more uniform. However I weekly hold this stance, and I think I care more about it in SQLite than I do other dialects, so maybe we can provide some way to tell the compiler not to care (that way we dont break existing code too)

AlecKazakova avatar Jul 11 '22 14:07 AlecKazakova

We could just add %ignorecase to the lexer.

hfhbd avatar Sep 26 '22 07:09 hfhbd

i think that would still have the issue I posted above, if its at the lexer level than lowercase update would get tokenized and be unusable for an identifier

AlecKazakova avatar Sep 26 '22 16:09 AlecKazakova

It would be very helpful if it can be optionally supported.

It is very hard for me to read SQL that is all uppercase. Writing is OK because IntelliJ plugin suggests even on lowercase and change the case accordingly, but reading is so much more frequent than writing.

The uppercase is maybe little bit helpful when viewing SQL like plain text, but with IDE hightlighting it is just distracting and hard readable, with longer definitions of many consequent keywords.

Personally, the tradeoff that we cannot use SQL keywords as labels in this optional mode is worth it.

morki avatar Oct 21 '22 08:10 morki

Here is a twitter poll about SQL casing from Lukas Eder, the creator of jOOQ, just for context. https://twitter.com/lukaseder/status/1052809813447593986

morki avatar Oct 21 '22 08:10 morki

Are there any plans to support this feature?

DuchGhast avatar Apr 05 '23 15:04 DuchGhast