Keyword handling

[manticore-projects]

Coming from JSQLParser, I know that keyword handling in SQL is a nightmare.
As far as I understand it, every Token of the Grammar must be white listened or it becomes a Reserved Keyword automatically.

What I have done for JSQLParser is:

1. Define a set of SQL:2016 based Reserved Keywords
2. Then extract all defined Tokens from the Grammar into a "ALL KEYWORDS" list
3. Then brute-forcefully run tests, where I used "ALL KEYWORDS" as schema, table, column names, aliases, function names etc.
   When it fails, I add it to the Reserved Keywords. When it succeeds, I add it to the WhiteList production in the grammar.

In the result I get a proper documentation of the actual Reserved Keywords and the Grammar gets properly adjusted when new tokens or productions are added.

Caveats:
a) the Tokens are extracted from the Grammar via Regular Expressions, which does not work well for compound tokens.
(I have an implementation using JavaCC, but that would add a dependency.)

b) updating the Grammar semi-automatically works only when the WhiteList Production is locked and we agree that it should not be edited manually.

Pros:
a) less error reports, this has reduced the reported issues by 50 %.
b) new contributors don't need to worry about keyword handling (when they tweak productions and add features)

Would something like this be useful for this project? Should I craft a PR? (Its a lot of work so I need to test the waters.)

[manticore-projects]

Illustration of my concern:

SELECT Overlaps( overlaps ) AS overlaps
FROM overlaps.overlaps overlaps
WHERE overlaps = 'overlaps'
    AND (CURRENT_TIME, INTERVAL '1' HOUR) OVERLAPS (CURRENT_TIME, INTERVAL -'1' HOUR)
;

From here we go straight to "semantic syntax highlighting" and LSP support :-D

[kaikalur]

in any RDBMS I know (see my example above).

If so, do it in JSQL. We will keep this grammar pure.

[manticore-projects]

If so, do it in JSQL. We will keep this grammar pure.

Thanks for the clarification, I get your point.
So everything in ReservedKeywords will be strictly reserved. I needed that for understanding and documentation purpose only.
Cheers.

[kaikalur]

If so, do it in JSQL. We will keep this grammar pure.

Thanks for the clarification, I get your point. So everything in ReservedKeywords will be strictly reserved. I needed that for understanding and documentation purpose only. Cheers.

Yeah, as you can see this grammar is mechanically generated from the spec :) so I had no idea (until now) OVERLAPS and SQRT (and many others) were reserved keywords.

[manticore-projects]

Note to myself:

1. I quickly moved OVERLAPS to the NonReservedKeywords and this made the Sample above parsing correctly. So the mechanism works in general.
2. We will need to keep a pure Grammar as requested and derive some "practical" Grammars from it (for parsing the actual dirty sql statements roaming the jungle). I need to figure out a smart way to switch between Grammars/Fragments on demand.

[kaikalur]

Note to myself:

1. I quickly moved OVERLAPS to the NonReservedKeywords and this made the Sample above parsing correctly. So the mechanism works in general.

Cool!

2. We will need to keep a pure Grammar as requested and derive some "practical" Grammars from it (for parsing the actual dirty sql statements roaming the jungle). I need to figure out a smart way to switch between Grammars/Fragments on demand.

I think you can simply relax the identifier rule to allow reserved words also as identifiers (using a flag) so no need to fork/copy etc. Just a flag that relaxes the spec rules.

[kaikalur]

Also, TestKeywords which is part of the standard test suite, does this automatically to make sure when you add a new keyword, it's not reserved word.

[manticore-projects]

Also, TestKeywords which is part of the standard test suite, does this automatically to make sure when you add a new keyword, it's not reserved word.

Thanks for all your support and patience! I do appreciate!

[manticore-projects]

Lol, I just broke your test by adding a token to NonReservedKeywords. It broke the counter and let the COUNT Token test fail!

Only when I add the Token before the Count Token, it works as expected.
Please allow me to modify this particular test and to actually parse the NonReservedKeywords -- independent from any order, white space and start/stop tokens.

[kaikalur]

That's by design. You should add any no reserved before MAX_NON_RESERVED Get Outlook for Android<https://aka.ms/AAb9ysg>

…

________________________________ From: manticore-projects ***@***.***> Sent: Tuesday, December 27, 2022 8:42:18 AM To: prestodb/sql ***@***.***> Cc: Sreeni Viswanadha ***@***.***>; Comment ***@***.***> Subject: Re: [prestodb/sql] Keyword handling (Discussion #33) Lol, I just broke your test by adding a token to NonReservedKeywords. It broke the counter and let the COUNT Token test fail! Only when I add the Token before the Count Token, it works as expected. Please allow me to modify this particular ZjQcmQRYFpfptBannerStart This Message Is From an External Sender ZjQcmQRYFpfptBannerEnd Lol, I just broke your test by adding a token to NonReservedKeywords. It broke the counter and let the COUNT Token test fail! Only when I add the Token before the Count Token, it works as expected. Please allow me to modify this particular test and to actually parse the NonReservedKeywords -- independent from any order, white space and start/stop tokens. — Reply to this email directly, view it on GitHub<#33 (reply in thread)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AAPNYANYFG4STTMSIW5H243WPML6VANCNFSM6AAAAAATKN7M3Y>. You are receiving this because you commented.Message ID: ***@***.***>

[kaikalur]

So the recipe here is that reserved and non-reserved words are in two sections and should be contiguous. If there is some bug in the test we should fix it but I don't see the need for the logic to change. We may also need to make the final test (for extraneous keywords) more robust. The test failure is legit.

[manticore-projects]

Agreed on the receipt and the sound logic. (I did pretty much the same thing in JSQLParser, just less elegant.)

I would like to suggest the following changes only:

1. instead depending on the start/stop tokens, get the actual tokens from the grammar and feed them into the test
2. test the tokens for more SQL Object identifiers such as schema-, table-, column-names, function names, aliases -- as part of the select items, CTEs, where, order, aggregate functions.
3. write the found/tested tokens onto the Sphinx website

I will craft a separate PR.

[kaikalur]

You can just call the identifier() rule (I think) to test something is accepted as an identifier or not. We don't need separate tests. I like to keep this compact. We want to test only the fact that non-reserved are accepted as identifiers and reserved ones are rejected as identifiers and they are in the right place in the grammar token blocks.

[kaikalur]

Actually just add an interface that checks if a given token is a reserved word or not and override it in different parsers to return different values. that's the easiest way