feat(webui): Set up ANTLR tooling for future client-side SQL parsing; Move clp-s parser generation to codegen.yaml; Remove unused client package-lock.json.#1293
Conversation
WalkthroughAdds a SQL parser to the web UI using ANTLR4, including a new grammar, TypeScript validation module, and build tasks for code generation. Updates ESLint to ignore generated code, adds the antlr4 dependency, adjusts Taskfile includes and CI workflow to use centralized codegen tasks, and updates NOTICE. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant Validator as sql-parser/index.ts
participant CharStream as UpperCaseCharStream
participant Lexer as SqlBaseLexer (ANTLR)
participant Parser as SqlBaseParser (ANTLR)
participant Listener as SyntaxErrorListener
Caller->>Validator: validate(sqlString)
Validator->>CharStream: wrap input (uppercasing)
Validator->>Lexer: new Lexer(CharStream)
Validator->>Listener: attach to Lexer and Parser
Validator->>Parser: new Parser(TokenStream(Lexer))
Parser->>Parser: singleStatement()
Parser-->>Listener: reportError? (on syntax issue)
alt syntax error
Listener-->>Validator: throw SyntaxError(line:col msg)
Validator-->>Caller: propagate SyntaxError
else valid
Parser-->>Validator: complete
Validator-->>Caller: return (void)
end
sequenceDiagram
autonumber
actor Dev/CI
participant Task as task webui-generate-parser
participant ANTLR as java -jar antlr.jar
participant FS as Filesystem
participant Utils as checksum/replace-text
Dev/CI->>Task: invoke
Task->>Utils: checksum validate (OUTPUT_DIR)
Task->>FS: rm -rf generated
Task->>ANTLR: generate TS from SqlBase.g4 (-no-listener)
Task->>FS: prune non-*.ts
Task->>Utils: prepend // @ts-nocheck to lexer/parser
Task->>Utils: checksum compute
Task-->>Dev/CI: done
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (5)📚 Learning: 2025-06-02T18:22:24.060ZApplied to files:
📚 Learning: 2025-05-27T20:06:21.263ZApplied to files:
📚 Learning: 2025-09-10T22:13:51.907ZApplied to files:
📚 Learning: 2025-08-09T04:07:27.083ZApplied to files:
📚 Learning: 2025-05-28T18:33:30.155ZApplied to files:
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| "start": "vite", | ||
| "guided": "VITE_GUIDED_DEV=true npm run start" | ||
| "guided": "VITE_GUIDED_DEV=true npm run start", | ||
| "generate-parser": "cd src/SqlParser && antlr4 -Dlanguage=TypeScript -o generated -no-listener -Xexact-output-dir SqlBase.g4 && find generated -type f -and -not -name '*.ts' -exec rm {} \\; && sed -i '1s/^/\\/\\/ @ts-nocheck\\n/' generated/*.ts" |
There was a problem hiding this comment.
I feel like we should simplify this script. I think you can just ignore the typescript in the tsconfig file instead of injecting the ignore. Also why are we deleting these other files? Why not just leave them?
There was a problem hiding this comment.
Also maybe we should add a short readme into the sql Parser folder reminding people to install pip3 install antlr4-tools. Since this is needed?
There was a problem hiding this comment.
Also it may make sense to put this script in the readme, instead of package.json, since it isnt actually using the npm dependency. Its not like it will be run often
There was a problem hiding this comment.
I guess "exclude": ["...."] won't work in tsconfig. Is any other config I'm not aware of?
Deleting unused files are what clps do for kql files, and it also has a similar script in the taskfile.
I guess a proper solution is to move this solution to taskfile?
There was a problem hiding this comment.
for the // @ts-nocheck, I think you can make a new tsconfig like tsconfig/tsconfig.generated.json and have it include "include": [
"../src/sql-parser/generated/**/*"
],
and have tsconfig/tsconfig.app.json exclude it. Then have the main tsconfig look like this
"references": [
{
"path": "./tsconfig/tsconfig.app.json"
},
{
"path": "./tsconfig/tsconfig.node.json"
},
{
"path": "./tsconfig/tsconfig.generated.json"
}
]
}
There was a problem hiding this comment.
Okay setup looks great. Added my comments. I think we will need another PR after this to do individual field parsing.
Last thing is I think we need to copy the Presto NOTICE file into our repo per license. And optionally give atttribution in there.
Im not that familiar with the licensing stuff so maybe we ask kirk
codegen.yaml.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
code gen looks good to me. @kirkrodrigues can you just double check the checksum stuff in the taskfile. I dont really know well how that works, and would prefer if someone familiar with the taskfiles checked.
@hoophalab
i added comments on the ts-ignore, as well as the uppercase.
Last thing - I think we should create an issue about adding workflow in the future like clp-s
|
title: |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
davemarco
left a comment
There was a problem hiding this comment.
latest changes look good to me
codegen.yaml; Remove unused client package-lock.json.
junhaoliao
left a comment
There was a problem hiding this comment.
the PR title is good
i reviewed the taskfiles and posted some stylistic comments; otherwise this is good to good
Co-authored-by: Junhao Liao <junhao@junhao.ca>
Co-authored-by: Junhao Liao <junhao@junhao.ca>
Co-authored-by: Junhao Liao <junhao@junhao.ca>
Co-authored-by: Junhao Liao <junhao@junhao.ca>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
taskfile.yaml(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-05-28T18:33:30.155Z
Learnt from: anlowee
PR: y-scope/clp#925
File: taskfiles/deps/main.yaml:97-106
Timestamp: 2025-05-28T18:33:30.155Z
Learning: In the taskfiles dependency system (taskfiles/deps/main.yaml), echo commands are used to generate .cmake settings files that are consumed by the main CMake build process. These files set variables like ANTLR_RUNTIME_HEADER to point to dependency locations for use during compilation.
Applied to files:
taskfile.yaml
🔇 Additional comments (1)
taskfile.yaml (1)
7-7: Centralizing parser generation via codegen include — verifiedclp-s-generate-parsers is present in taskfiles/codegen.yaml and not marked internal (no 'internal: true' found).
junhaoliao
left a comment
There was a problem hiding this comment.
let's fix one nit then this is good to go
Co-authored-by: Junhao Liao <junhao@junhao.ca>
… Move clp-s parser generation to `codegen.yaml`; Remove unused client `package-lock.json`. (y-scope#1293)
Description
https://github.com/prestodb/presto/blob/release-0.293/presto-parser/src/main/antlr4/com/facebook/presto/sql/parser/SqlBase.g4
permalink: https://github.com/prestodb/presto/blob/2f48ff63a3d6d249b4062546003126ddbbb989ee/presto-parser/src/main/antlr4/com/facebook/presto/sql/parser/SqlBase.g4
Checklist
breaking change.
Validation performed
validatedetects incorrect sql queries.clp-s-generate-parsersworks the same as before.webui-generate-parsergenerates the parser correctly.Summary by CodeRabbit
New Features
Chores