Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: upgrade trino autocomplete #247

Merged
merged 5 commits into from
Nov 22, 2024
Merged

Conversation

NikitaShkaruba
Copy link
Collaborator

No description provided.

@NikitaShkaruba NikitaShkaruba self-assigned this Nov 21, 2024

# Intellij IDES add weird token files that we don't use
**/grammar/*.tokens
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IDK how to fix it and have the antlr syntax highlighting, decided to just ignore this file


statements
: statement SEMICOLON_?
| statement SEMICOLON_ statements
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Current Trino client doesn't work with semicolons and multiple statements, so I've removed it from our autocomplete for now for better UX

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we have special logic for Trino for this? ClickHouse also doesn't support multiqueries but we have it in autocomplete
I don't think our autocomplete library should be concerned with how our websql code works, it should be independent
Also when we soon support this we will have to add it again

We should just throw error on multiple query execution like we do in ClickHouse

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with Robert, need to discuss this decision

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We've discussed it. Decided to go for better UX for now

: rootQuery # statementDefault
| USE_ schema = identifier # use
| USE_ catalog = identifier DOT_ schema = identifier # use
| CREATE_ CATALOG_ (IF_ NOT_ EXISTS_)? catalog = identifier USING_ connectorName = identifier (
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Basically I've splitted a bunch of simple identifiers to tableIdentifier, schemaIdentifier, catalogIdentifier to have a simple autocomplete


viewIdentifier
: tableIdentifier
;
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trino-client doesn't allow for USE statements, so we are forced to always give a fully qualified name like catalog.schema.table. This will greatly improve UX, because otherwise user will need to get an error from back end

@NikitaShkaruba NikitaShkaruba merged commit 350c1d3 into main Nov 22, 2024
4 checks passed
@NikitaShkaruba NikitaShkaruba deleted the WEBSQL-212/richer_autocomplete branch November 22, 2024 11:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants