[feat] Support for JSON_TABLE#2328
Conversation
manticore-projects
left a comment
manticore-projects
left a comment
There was a problem hiding this comment.
Great work and I do appreciate your effort and dedication. Thank you much!
A few small concerns though, nothing severe just the normal German nit-picking.
Kudos and cheers!
| | <#TYPE_REAL: "REAL" | "FLOAT4" | "FLOAT"> | ||
| | <#TYPE_DOUBLE: "DOUBLE" | "PRECISION" | "FLOAT8" | "FLOAT64"> | ||
| | <#TYPE_VARCHAR: "NVARCHAR" | "VARCHAR" | "NCHAR" | <K_CHAR> | "BPCHAR" | "TEXT" | "STRING" | <K_CHARACTER> | "VARYING"> | ||
| | <#TYPE_VARCHAR2: <K_VARCHAR2>> |
There was a problem hiding this comment.
This should got to TYPE_VARCHAR since we try to establish a reasonable grouping please.
There was a problem hiding this comment.
Yeah, that was left over from me figuring it out
There was a problem hiding this comment.
This is a problem. When I move this into the TYPE_VARCHAR, the generated TokenManager gets a code too large. I guess we're reaching the limits of JavaCC again...
I'll have to look into why exactly this happens, but I've not applied this change yet in my last commit.
src/main/java/net/sf/jsqlparser/util/validation/validator/ExpressionValidator.java
Outdated
Show resolved
Hide resolved
src/main/java/net/sf/jsqlparser/util/deparser/SelectDeParser.java
Outdated
Show resolved
Hide resolved
src/main/java/net/sf/jsqlparser/util/deparser/ExpressionDeParser.java
Outdated
Show resolved
Hide resolved
src/main/java/net/sf/jsqlparser/statement/select/FromItemVisitorAdapter.java
Show resolved
Hide resolved
src/main/java/net/sf/jsqlparser/expression/JsonQueryWrapperType.java
Outdated
Show resolved
Hide resolved
src/main/java/net/sf/jsqlparser/expression/ExpressionVisitorAdapter.java
Outdated
Show resolved
Hide resolved
Keep the nit-picking coming :) There's still some features missing before I'll consider this PR ready for merge. |
@ANeumann82 Great Work!! Can you please let me know the ETA for this change |
|
@ANeumann82: How can we move this forward together? What assistance can I provide please? |
# Conflicts: # src/main/java/net/sf/jsqlparser/util/deparser/ExpressionDeParser.java # src/main/jjtree/net/sf/jsqlparser/parser/JSqlParserCC.jjt
|
@manticore-projects Sorry for the long silence. I've been into a rabbit hole of JavaCC and other parsers... I've updated the PR from the master branch, but now I get "code too large" in the generated "CCJsqlParserTokenManager.java". Do you have any idea to work around that problem? I don't know if there's anything we can do about that with the current JavaCC version. I'm currently looking into alternative parsers to maybe replace JavaCC in JSQLParser at some point, but that's not easy. |
Have done that, have been there and you are welcome.
I will lodge a case with the JavaCC team, maybe they can help.
We have too many token apparently. I want to check first if the JavaCC team can help.
I am most interested in this one too and dug deep already. Unfortunately there is not really any good alternative, but maybe |
I am reworking the Tokens and the Reserved Keywords as we speak to free up the token manager. |
|
Greetings @ANeumann82, as promised I have completely reworked the token handling. So we should be able to proceed with your PR please. |
|
Haha, thanks for the ping! :) I only took a quick look through this PR, but my main concern is more about the overall direction than the style details. Current master already seems to model JSON_TABLE through TableFunction/JsonTableFunction together with newer token handling, while this branch introduces a separate JsonTable FromItem path and expands the older token setup again. It may be worth considering whether the remaining JSON_TABLE support here could be folded into the existing master-side model instead of keeping two parallel representations. That looks like it could simplify the visitor/deparser/validator side and may also help avoid running back into the older TokenManager size issues. |
4 participants
No description provided.