Add all "type reserved" MySQL keywords #106

mvorisek · 2024-05-13T17:04:34Z

Into 1.4.x as this is bugfix.

Also add reserved keyword lists for major DB vendors for future "non-reserved" (keywords that do not have to be quoted) lists compilation.

SenseException · 2024-05-30T18:40:04Z

Please address the color/format changes in the test output and how these are necessary for adding reserved keywords in this PR.

mvorisek · 2024-06-01T17:34:07Z

Please address the color/format changes in the test output and how these are necessary for adding reserved keywords in this PR.

@SenseException Here is an explanation - some keywords like TINYINT were not present, but they are reserved, so they were added, and thus the highlighting has changed. This is expected and all reserved keywords are listed for each DB in the TokenizerTest and tested if the Tokenizer lists are defined as such.

Is this explaining your question or do you observed anything specific you want to have answered more in depth?

greg0ire · 2024-06-01T18:42:25Z

src/Tokenizer.php

@@ -22,62 +23,57 @@
 final class Tokenizer
 {
    /**
-     * Reserved words (for syntax highlighting)
+     * Reserved MySQL and MariaDB keywords


When reading that, one might think they shouldn't add keywords for any other DB… is that the case?

Yes - and this is also strictly tested using testKeywordsReservedAreStrictlyMysqlAndMariadbReserved test. The reason is we must be sure the "reserved" keywords are really "reserved", as only these keywords can be uppercased safely - for #32. All other keywords must be put into "non-reserved" list, even if they are "reserved" in other than MySQL/MariaDB DB vendor.

In practise, this is very clever, as generic SQL keywords (like WHERE, UPDATE, ...) are reserved everywhere and exotic keywords can be still highlighted using the "non-reserved" list but their case must be kept to prevent #95.

Are there keywords that could be reserved according to MariaDB, but not according to other vendors? I don't get why MySQL/MariaDB is privileged.

The reasons are historical, jdorn library was based on MySQL keywords solely.

I already added full SQLite reserved keywords list to the tests so all of them are newly contained in the "non-reserved" list.

This PR aims to add MySQL/MariaDB/SQLite keywords. Other vendors can be added later.

For highlighting, "reserved"/"non-reserved" lists are used the same way. For #32, we need "reserved" list to be minimal subset (intersect) of all vendors.

Are there keywords that could be reserved according to MySQL/MariaDB, but not according to other vendors? If yes, what did you do with these keywords?

When I ask a question, answer that question. Don't answer another question, or if you don't know, just say so. So for the third time, "Why did you make me ask 3 times?" I want a honest and plausible answer, is that too much to ask?

I tried to answer your question in my 1st answer with some explanation...

So are we on the same page now? Is here anything else actionable I should amend in this PR?

I tried to answer your question in my 1st answer with some explanation...

Really? Let's see…

The question was:

Are there keywords that could be reserved according to MySQL/MariaDB, but not according to other vendors?

You answered:

The reasons are historical, jdorn library was based on MySQL keywords solely.

This feels more like an answer to the second question, which was:

I don't get why MySQL/MariaDB is privileged.

Then you went on with:

I already added full SQLite reserved keywords list to the tests so all of them are newly contained in the "non-reserved" list.

Still aimed at the second question

This PR aims to add MySQL/MariaDB/SQLite keywords. Other vendors can be added later.

Still aimed at the second question as well

For highlighting, "reserved"/"non-reserved" lists are used the same way. For #32, we need "reserved" list to be minimal subset (intersect) of all vendors.

Interesting, but does not have to do with any of the 2 questions.

So are we on the same page now?

Well… no! You said you tried to answer your question in your first answer, but I think I just demonstrated that you completely ignored it. It feels like we live in separate realities.

Is here anything else actionable I should amend in this PR?

One thing at a time please. Before I invest more time in this PR, I'd like to understand if you are deliberately wasting my time or not.

@greg0ire Thank you your feedback helped me to realize few important facts and I have reworked this PR.

I have removed the ambiguous "reserved" vs "non-reserved" categorization and added exact "reserved" keyword list for each major DB vendor.

Let me please know what you think and if I should possibly extract some commit into separate PR.

Minimize diff for next commits, non functional change.

Functions are always case insensitive and cannot contain a whitespace.

"reserved" keyword is tokenized sooner thus the same keyword defined as "function" is useless.

To fix all type highlighting in function tests.

For future compilation of Tokenizer lists. Also test if all intersected keywords are really reserved.

greg0ire · 2024-06-02T21:36:40Z

tests/TokenizerTest.php

+    ];
+
+    /**
+     * Based on https://www.postgresql.org/docs/16/sql-keywords-appendix.html list.


That link looks like a copy/paste, but maybe that's normal? First time I hear about sql2023.

The PostgreSQL link is intended there as the SQL:2023 spec is non-public/paid - I find PostgreSQL as quite reputable source and then I cross verified it againts https://en.wikipedia.org/wiki/List_of_SQL_reserved_words. Is the actual comment fine or should I update it in any way?

It's fine, just wanted to make sure.

mvorisek force-pushed the more_reserved_kws branch 5 times, most recently from 807424e to dd64ad1 Compare May 17, 2024 12:41

mvorisek changed the title ~~Add reserved SQLite keywords~~ Add all reserved MySQL/MariaDB reserved keywords May 17, 2024

mvorisek changed the title ~~Add all reserved MySQL/MariaDB reserved keywords~~ Add all reserved MySQL/MariaDB keywords May 17, 2024

mvorisek force-pushed the more_reserved_kws branch 3 times, most recently from c51682a to 3af69d8 Compare May 17, 2024 19:37

mvorisek changed the title ~~Add all reserved MySQL/MariaDB keywords~~ Add all reserved MySQL/MariaDB/SQLite keywords May 17, 2024

mvorisek mentioned this pull request May 17, 2024

Make sure keywords are single upper words #107

Merged

mvorisek force-pushed the more_reserved_kws branch 2 times, most recently from 8c20a13 to 13a1826 Compare May 21, 2024 10:29

mvorisek mentioned this pull request May 22, 2024

Make sure all internal lists do not rely on keywords order #117

Merged

mvorisek force-pushed the more_reserved_kws branch 2 times, most recently from 19b74f2 to f0d6aab Compare May 26, 2024 06:36

mvorisek force-pushed the more_reserved_kws branch 2 times, most recently from 3000378 to 33ba5c0 Compare May 30, 2024 16:36

mvorisek marked this pull request as ready for review May 30, 2024 16:37

greg0ire reviewed Jun 1, 2024

View reviewed changes

mvorisek force-pushed the more_reserved_kws branch from 33ba5c0 to edc05c4 Compare June 1, 2024 22:42

mvorisek requested a review from greg0ire June 2, 2024 08:24

mvorisek marked this pull request as draft June 2, 2024 15:19

mvorisek added 2 commits June 2, 2024 18:36

Add "reserved" words from top level/new line lists

a19a0a5

Minimize diff for next commits, non functional change.

Improve assertion for "function" keyword list

5219b17

Functions are always case insensitive and cannot contain a whitespace.

mvorisek force-pushed the more_reserved_kws branch 2 times, most recently from c843eba to 1cfcac2 Compare June 2, 2024 17:04

mvorisek added 2 commits June 2, 2024 19:09

Assert "function" keyword is not "reserved"

82e01b2

"reserved" keyword is tokenized sooner thus the same keyword defined as "function" is useless.

Add all "type reserved" MySQL keywords

0f7b39d

To fix all type highlighting in function tests.

mvorisek force-pushed the more_reserved_kws branch 2 times, most recently from 26064d6 to b34f582 Compare June 2, 2024 17:55

mvorisek changed the title ~~Add all reserved MySQL/MariaDB/SQLite keywords~~ Add all "type reserved" MySQL keywords Jun 2, 2024

Add exact reserved keyword lists into TokenizerTest

1c0187f

For future compilation of Tokenizer lists. Also test if all intersected keywords are really reserved.

mvorisek force-pushed the more_reserved_kws branch from b34f582 to 1c0187f Compare June 2, 2024 17:59

mvorisek marked this pull request as ready for review June 2, 2024 18:04

greg0ire reviewed Jun 2, 2024

View reviewed changes

greg0ire approved these changes Jun 3, 2024

View reviewed changes

greg0ire requested review from goetas, SenseException and derrabus June 3, 2024 06:04

SenseException approved these changes Jun 3, 2024

View reviewed changes

greg0ire added this to the 1.4.1 milestone Jun 3, 2024

greg0ire merged commit 7c0096a into doctrine:1.4.x Jun 3, 2024
10 checks passed

mvorisek deleted the more_reserved_kws branch June 3, 2024 21:19

greg0ire added the bug Something isn't working label Aug 5, 2024

github-actions bot mentioned this pull request Aug 5, 2024

Merge release 1.4.1 into 1.5.x #135

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add all "type reserved" MySQL keywords #106

Add all "type reserved" MySQL keywords #106

mvorisek commented May 13, 2024 •

edited

Loading

SenseException commented May 30, 2024

mvorisek commented Jun 1, 2024

greg0ire Jun 1, 2024

mvorisek Jun 1, 2024 •

edited

Loading

greg0ire Jun 1, 2024

mvorisek Jun 1, 2024

greg0ire Jun 2, 2024 •

edited

Loading

greg0ire Jun 2, 2024

mvorisek Jun 2, 2024

greg0ire Jun 2, 2024

This comment was marked as resolved.

mvorisek Jun 2, 2024

greg0ire Jun 2, 2024 •

edited

Loading

mvorisek Jun 2, 2024

greg0ire Jun 3, 2024

Add all "type reserved" MySQL keywords #106

Add all "type reserved" MySQL keywords #106

Conversation

mvorisek commented May 13, 2024 • edited Loading

SenseException commented May 30, 2024

mvorisek commented Jun 1, 2024

Choose a reason for hiding this comment

mvorisek Jun 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greg0ire Jun 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

greg0ire Jun 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mvorisek commented May 13, 2024 •

edited

Loading

mvorisek Jun 1, 2024 •

edited

Loading

greg0ire Jun 2, 2024 •

edited

Loading

greg0ire Jun 2, 2024 •

edited

Loading