Enhanced Keywords #1382

manticore-projects · 2021-10-18T14:43:16Z

Rewrite the Reserved Keywords Logic:

Automatically Derive all Keyword Tokens from the Parser.
Explicitly add Reserved Keywords to the Grammar and classify, why/when it is reserved.
Auto-generate the list of Allowed Keywords as Difference of 1) and 2) above
Provide Gradle task to semi-auto generate RelObjectNameWithoutValue() based on 3)
Parametrized Tests for Keywords, testing all keywords

Advantage:
a) we have now a clear documentation which Keywords are reserved for what reason, with proper tests
b) allowed keywords are generated (semi-)automatically, especially when more Tokens are added to the Grammar. New Tokens won't break SQL Statements, which have been parsed fine before.

To-Dos:
c) Composite Tokens do not work well and still need to be added manually to ALL_KEYWORDS (we would need to refactor the GRAMMAR in order to avoid such composite tokens)
d) @wumpz: Clarify the meaning/purpose of the various RelObjectNamexxxx() Productions, so we can auto generate them too.
e) Use the Gradle task to fully inject the RelObjectNamexxxx() Productions (instead of manually updating the code)

Resolves one more Special Oracle Test.
Fixes #1148
Fixes #1450
Fixes #1443
Fixes #1462
Fixes #1508
Fixes #1538
Fixes #1650

Add Keywords and document, which keywords are allowed for what purpose

wumpz

Again, are all those keyword additions to relobjectnames needed? You talked about all those edge case improvements. Are most of those additions not exactly that?

src/main/jjtree/net/sf/jsqlparser/parser/JSqlParserCC.jjt

wumpz · 2021-10-20T21:47:23Z

src/main/jjtree/net/sf/jsqlparser/parser/JSqlParserCC.jjt

@@ -1733,7 +2007,7 @@ Table TableWithAlias():
    Alias alias = null;
 }
 {
-    table=Table() [alias=Alias() { table.setAlias(alias); }]
+    table=Table() [ LOOKAHEAD(2) alias=Alias() { table.setAlias(alias); }]


It should be the aim to do less not more LOOKAHEADs. All this for some keywords?

Yes, all this for some keywords (although the Alias() Production is affected mostly)
But why exactly would you deny Tokens, which are not defined in the holy SQL:2016 standard as keywords when there is no compelling technical reason ?

Complaints about keywords are felt the second most concern of the users, right after quoting/escaping text and objects.

To me it's not directly obvious why this LOOKAHEAD is needed. Is that explained somewhere?

src/test/java/net/sf/jsqlparser/statement/KeywordsTest.java

Derive All Keywords from Grammar directly Generate production for Object Names (semi-) automatically Add parametrized Keyword Tests

manticore-projects · 2021-11-01T13:22:32Z

@wumpz: Please check out the great work on PR #1254. It adds a new token QUICK and which of course should not become a Reserved Keyword.

Of course the author @OlivierCavadenti has no chance to know, that he would need to add QUICK to the productions RelObjectNamexxxx() in order to enable this token as Object Identifier. (And how should he, I figured that out only 6 month after my first contribution.)

This PR would solve this challenge reliably, preventing new tokens from breaking valid statements and ease the entry for new contributors.

Parallel Test execution Gradle Caching Explicitly request for latest JavaCC 7.0.10

# Conflicts: # src/main/jjtree/net/sf/jsqlparser/parser/JSqlParserCC.jjt

Update keywords

wumpz · 2021-11-28T21:55:48Z

build.gradle

@@ -227,6 +242,13 @@ task renderRR() {
        } 
    }
 }
+
+task updateKeywords(type: JavaExec) {


Since I do not use gradle, what does this do?

Maven is the main build engine. You changed some gradle build options. Does the maven build still run?

It generates and prints the Source Code Text of the method RelObjectName() which you would manually insert/replace in the JSQL Grammar file.

This step is optional and does not affect the Maven build. It is executed manually and on demand only.

Although the long term goal was to have a mechanism inside the build tool (Gradle and/or Maven), which during the build:

analyses the KeyWords automatically

modifies the Grammar file (methods RelObjectName() and friends)

builds the Parser with the modified Grammar

runs the Keyword Tests

I think this might also be implemented in a mojo in Maven, but that might be not worth the effort.

wumpz · 2021-11-28T22:00:27Z

The naming of this RelObjectName ... methods is sure an issue that should be adressed in futher improvements. So sorry for that: its grown ... ;)

wumpz · 2021-11-28T22:04:07Z

I am not a fan of injecting those keywords, since not every keyword could be introduced this way and needs other parts of the grammar to be modified. However, building this keyword testing using JUnit 5 is cool.

manticore-projects · 2021-11-29T07:59:27Z

The naming of this RelObjectName ... methods is sure an issue that should be adressed in futher improvements. So sorry for that: its grown ... ;)

You would do me a great favor by documenting the purpose of those 5 methods verbosely. I tried my best to figure it out by trial'n error but I am still not exactly sure why we ended up with 5 methods. (And no worries about grown code.)

manticore-projects · 2021-11-29T08:09:23Z

I am not a fan of injecting those keywords, since not every keyword could be introduced this way and needs other parts of the grammar to be modified. However, building this keyword testing using JUnit 5 is cool.

Honestly, I am not a big fan of that PR either -- but after spending many days on that matter trying various things it was the best approach I found and in my opinion it is still much better than the current state:

at the moment, Keywords are just a mess and we get a lot of issues about keywords only.
Are you able to document right now, what keywords are reserved and for what reason? I do not think so and the PR would solve this documentation problem.
today, even when you have an opinion on keywords, we have no parametrized tests.
You kindly admit, that these tests are kind of cool and I appreciate your feedback. Those parametrized tests however depend on a well defined list of keywords, which the PR provides. (Although we can argue of course, how/where exactly such a list should be defined.)

not every keyword could be introduced this way and needs other parts of the grammar to be modified

While that is true for the PR, it is also true by now already. Also we could easily enforce a policy of "Keywords are defined by simple Tokens" (not allowing complex Tokens for Keywords).

The work flow is to: a) define the Tokens and Productions in the Grammar first and then b) document any new RESERVED Keywords only in the List (emphasis on RESERVED).

The PR has one big advantaged: By defining RESERVED keywords only, it will determine automatically any allowed Keywords and modify the Grammar semi-automatically.

manticore-projects · 2021-11-29T08:15:39Z

Allow me to ask please:

what was your preferred way to manage and document RESERVED KEYWORDs
what was your preferred way to maintain any allowed Token/Keyword in RelObjectName()
what was your preferred way to test all the possible Keywords (automatically)

And can we have a Skype chat about this :-)

manticore-projects · 2022-09-21T05:34:36Z

Since you only have a list of all whitelisted keywords in ParserKeywordsUtil are not detected. So every addition of tokens needs to be recognized by a developer.

No, it is exactly the opposite: Restricted Keywords are rather static and would not be touched normally. Developer adds to All Keywords only by adding tokens/productions to the Grammar. Then the Gradle task builds Whitelist Keywords automatically and confirms them by brute force testing.

Only when those tests fail, we would have to amend Restricted Keywords. This could even be automated, it is simple Java Code after all (or could be a Text file or whatever.)

Where I still fail to understand you plan is this point:

We agree to run the brute force tests in order to identify/confirm Restricted Keywords, do we?
But those tests will work only AFTER updating the Whitelisted Keyword in RelObjectNameWithoutValue() -- because the tests rely on the final grammar.

So how would you run your Tests without whitelisting in the grammar first?

wumpz · 2022-09-25T23:52:52Z

So how would you run your Tests without whitelisting in the grammar first?

Simply this test will fail until you whitelist or restrict the new tokens. That's what forces developers to do this decision (for us). You are right, that the restricted list should change rarely. The test should then give an advice, how and were this change should be done.
I am a fan of brute force testing changes a developer (can) make. In a sense this is the same like enforcing a specific code style.

No, it is exactly the opposite: Restricted Keywords are rather static and would not be touched normally. Developer adds to All Keywords only by adding tokens/productions to the Grammar. Then the Gradle task builds Whitelist Keywords automatically and confirms them by brute force testing.

I was not aware of this automatic whitelist keyword building. So this is +1.

wumpz · 2022-10-16T20:41:54Z

So are you going to resolve the conflicts? This improvement of parsing could be done afterwards.

# Conflicts: # src/main/jjtree/net/sf/jsqlparser/parser/JSqlParserCC.jjt

vipcxj · 2022-10-24T01:00:50Z

@wumpz When could this pr be merged? The parse error is really annoying.

manticore-projects · 2022-10-24T01:40:24Z

@wumpz When could this pr be merged? The parse error is really annoying.

I can supply you with a bleeding edge JAR file including those PRs, in case its urgent.

wumpz · 2022-10-25T23:26:16Z

So after merging, I am not able to build anymore since ConditionalKeywordsTest fails. I merged another PR.

So how should I proceed if I am not willing to use gradle? Isn't that the same problem you had with my proposal? Maven is the main build pipeline.

manticore-projects · 2022-10-26T00:16:19Z

Good Morning. Thank you for finally accepting the PR.

What caused the problem:

you have accepted another PR before (introducing the new Token LOCKED)
you have not added LOCKED to the white- or black-list

Solution:
Amend the white- or black-list and then run the buildKeywords Task.

This should have been done by the author of the LOCKED PR from now on any author will be forced to do so.

It is exactly as you have requested for in your explanations.

Although I will run through this procedure today and send a very small PR.

manticore-projects · 2022-10-26T07:09:03Z

So how should I proceed if I am not willing to use gradle?

If you insist in not using gradle updateKeywords then you still can amend RelObjectNameWithoutValue manually and add the whitelisted new token -- same as it has always been done before.

I would also provide an equivalent Maven task soon, since we are moving forward now.

Workflow should be like that:

1) Amend Tokens in the Grammar
2) When keyword-tests fail, run gradle updateKeywords
3) Then build via mvn package or gradle build (it does not matter)
4) Push/Deploy

Using gradle updateKeywords task is optional, you can also add the new Tokens manually has it has always been done before.

After accepting the Keywords PR, any further PR will automatically fail acceptance tests, unless keywords are in order.

wumpz · 2022-10-28T22:23:39Z

No developer that is using maven knows what to do here? So this workflow has somehow to be included in a developer help. At the moment this is only your tool, since nobody knows and not everybody uses gradle.

wumpz · 2022-10-28T22:58:17Z

Since we are not willing to run the gradle task and your tests use ParserKeywordsUtils.getReservedKeywords how should updating the grammar be enough?

wumpz · 2022-10-28T23:17:15Z

Additional since some JavaCC methods are in place now there is a direct dependency for JavaCC.

manticore-projects · 2022-10-29T01:14:44Z

Additional since some JavaCC methods are in place now there is a direct dependency for JavaCC.

Yes, because you insisted to parse the Grammer using JavaCC instead of using Regular Expressions. I suggest to use Regular Expressions instead for that reason.

Since we are not willing to run the gradle task and your tests use ParserKeywordsUtils.getReservedKeywords how should updating the grammar be enough?

Because this is exactly what you would have done before the semi-automation: edit the Production RelObjectNameWithoutValue manually. You still could very much do that and we should keep that option at all cost.

That said, I have added a Maven Task for running the same updateKeywords task.
You can run it via:

mvn compile exec:java

No developer that is using maven knows what to do here? So this workflow has somehow to be included in a developer help. At the moment this is only your tool, since nobody knows and not everybody uses Gradle.

Yes, of course. It has been verbosely documented within the new website, which I have committed 1 month ago or so: https://manticore-projects.com/JSQLParser/contribution.html#manage-reserved-keywords

I have updated the Keywords2 PR #1653 accordingly with your requested Maven task.

d2a-raudenaerde · 2022-10-31T08:14:18Z

The Maven task could be implemented as a mojo (maven-plugin) I think, are you interested in that? Then you can run this plugin from the pom.xml during, for example, the generate-sources phase.

d2a-raudenaerde · 2022-10-31T08:15:34Z

I have some experience writing these maven plugins, so if I infd some time this week, maybe I could draft up some integration so it 'just works' when you run for example mvn compile

d2a-raudenaerde · 2022-10-31T08:35:17Z

But probably this will work as well:

<plugin>
        <groupId>org.codehaus.mojo</groupId>
        <artifactId>exec-maven-plugin</artifactId>
        <executions>
            <execution>
                <id>codegeneration</id>
                <phase>generate-resources</phase>
                <goals><goal>java</goal></goals>
                <configuration>
                   <mainClass>com.codegenerator.CodeGeneratorApplication</mainClass>
                </configuration>
            </execution>
        </executions>

</plugin>

manticore-projects · 2022-10-31T08:55:39Z

Thanks! I have add submitted another PR like that on Saturday already.

d2a-raudenaerde · 2022-10-31T10:00:31Z

Ah excellent!
I'll have to catch up I see :)

d2a-raudenaerde · 2022-10-31T11:41:00Z

I can't find that PR? I think there might also be a chicken-and-egg problem, as the java code that does the work, also needs to be compiled first? (you can solve that using multi-module maven build, but that requires a bit more refactoring)

manticore-projects · 2022-10-31T11:52:53Z

this one: #1653

Its pretty straight forward actually since we can execute Java Code directly without compiling the whole package. Also, this step is executed on demand: only when new tokens introduced and/or the Keyword Tests fail.

wumpz · 2022-11-02T22:38:29Z

Yes, because you insisted to parse the Grammer using JavaCC instead of using Regular Expressions. I suggest to use Regular Expressions instead for that reason.

That's why e.g. my proposal is only running in scope test. Then this dependency is not needed. I was talking about the IMHO right way to parse the grammar and not the context in which this parsing should happen. I thought this should be obvious. For the same reason, the JSqlParser jar should not include this generation class. It is part of the build process (I know you do not accept that, but I think it's evident.) So a quick solution would be to put all of it in the test folder and make this JavaCC dependency of scope test.

However, to get a clean build again use for now Regular Expressions.

Saying that, since this is a build step or could be one, it should be separated from JSqlParser source code and put in some kind of build helper module and then could be definitely used if JSqlparser is built. Now If you clone this project you will fail, since the generator class is not yet built.

I have updated the Keywords2 PR #1653 accordingly with your requested Maven task.

I have reviewed it already and found in this issue the needed information on how to run it, so I will merge it.

manticore-projects added 2 commits October 18, 2021 21:41

Enhanced Keywords

834dab1

Add Keywords and document, which keywords are allowed for what purpose

Fix incorrect tests

7a885a1

manticore-projects mentioned this pull request Oct 19, 2021

Unexpected token: apply #1148

Closed

wumpz reviewed Oct 20, 2021

View reviewed changes

manticore-projects added 3 commits October 23, 2021 14:23

Merge remote-tracking branch 'origin/master' into Keywords

1b69d25

Define Reserved Keywords explicitly

5d20d06

Derive All Keywords from Grammar directly Generate production for Object Names (semi-) automatically Add parametrized Keyword Tests

Fix test resources

df9c56e

OlivierCavadenti mentioned this pull request Nov 1, 2021

Add Delete / Update modifiers for MySQL #1254 #1396

Merged

manticore-projects added 10 commits November 22, 2021 14:09

Adjust Gradle to JUnit 5

de805ab

Parallel Test execution Gradle Caching Explicitly request for latest JavaCC 7.0.10

Do not mark SpeedTest for concurrent execution

d5a6dca

Remove unused imports

a9d0503

Adjust Gradle to JUnit 5

6dfa05f

Parallel Test execution Gradle Caching Explicitly request for latest JavaCC 7.0.10

Do not mark SpeedTest for concurrent execution

8f0bfe6

Remove unused imports

5cd0974

Merge remote-tracking branch 'origin/master'

5f11d3f

Merge remote-tracking branch 'origin/Keywords' into Keywords

2af3cd5

Merge remote-tracking branch 'origin/master' into Keywords

4bef952

# Conflicts: # src/main/jjtree/net/sf/jsqlparser/parser/JSqlParserCC.jjt

Keyword test adopt JUnit5

1fd56ac

Update keywords

manticore-projects requested a review from wumpz November 28, 2021 08:40

wumpz reviewed Nov 28, 2021

View reviewed changes

Merge Master

873dfd4

CheckStyle sanitation of method names

1a9db26

Merge https://github.com/JSQLParser/JSqlParser

3ab04b0

manticore-projects mentioned this pull request Dec 30, 2021

select database() #1450

Closed

manticore-projects added 6 commits October 17, 2022 03:45

Merge remote-tracking branch 'origin/master' into Keywords

6953d8b

# Conflicts: # src/main/jjtree/net/sf/jsqlparser/parser/JSqlParserCC.jjt

merge Master

a8ffe64

Merge branch 'master' into Keywords

1d4eb9e

build: JSQLParser is a build dependency

b6146cf

chore: Update keywords

b07d839

Merge remote-tracking branch 'manticore/Keywords' into Keywords

1e159ba

# Conflicts: # src/main/jjtree/net/sf/jsqlparser/parser/JSqlParserCC.jjt

manticore-projects mentioned this pull request Oct 21, 2022

JSqlParser can't parse DATABASE function in mysql. #1650

Closed

feat: add line count to output

b0d6218

wumpz merged commit 4863eb5 into JSQLParser:master Oct 25, 2022

wumpz mentioned this pull request Nov 20, 2022

enhanced keyword - corrections #1669

Closed

Uh oh!

Enhanced Keywords #1382

Enhanced Keywords #1382

Uh oh!

Conversation

manticore-projects commented Oct 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wumpz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

wumpz Oct 20, 2021

Choose a reason for hiding this comment

Uh oh!

manticore-projects Oct 21, 2021

Choose a reason for hiding this comment

Uh oh!

d2a-raudenaerde Aug 2, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

manticore-projects commented Nov 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wumpz Nov 28, 2021

Choose a reason for hiding this comment

Uh oh!

manticore-projects Nov 29, 2021

Choose a reason for hiding this comment

Uh oh!

d2a-raudenaerde Aug 2, 2022

Choose a reason for hiding this comment

Uh oh!

wumpz commented Nov 28, 2021

Uh oh!

wumpz commented Nov 28, 2021

Uh oh!

manticore-projects commented Nov 29, 2021

Uh oh!

manticore-projects commented Nov 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

manticore-projects commented Nov 29, 2021

Uh oh!

manticore-projects commented Sep 21, 2022

Uh oh!

wumpz commented Sep 25, 2022

Uh oh!

wumpz commented Oct 16, 2022

Uh oh!

vipcxj commented Oct 24, 2022

Uh oh!

manticore-projects commented Oct 24, 2022

Uh oh!

wumpz commented Oct 25, 2022

Uh oh!

manticore-projects commented Oct 26, 2022

Uh oh!

manticore-projects commented Oct 26, 2022

Uh oh!

wumpz commented Oct 28, 2022

Uh oh!

wumpz commented Oct 28, 2022

Uh oh!

wumpz commented Oct 28, 2022

Uh oh!

manticore-projects commented Oct 29, 2022

Uh oh!

d2a-raudenaerde commented Oct 31, 2022

Uh oh!

d2a-raudenaerde commented Oct 31, 2022

Uh oh!

d2a-raudenaerde commented Oct 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

manticore-projects commented Oct 31, 2022

Uh oh!

d2a-raudenaerde commented Oct 31, 2022

Uh oh!

manticore-projects commented Oct 18, 2021 •

edited

Loading

manticore-projects commented Nov 1, 2021 •

edited

Loading

manticore-projects commented Nov 29, 2021 •

edited

Loading

d2a-raudenaerde commented Oct 31, 2022 •

edited

Loading