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

add parser interface in engine #2486

Merged
merged 19 commits into from
May 9, 2024
Merged

add parser interface in engine #2486

merged 19 commits into from
May 9, 2024

Conversation

jennifersp
Copy link
Contributor

@jennifersp jennifersp commented May 1, 2024

This PR creates sql.Parser interface. This interface is defined in the engine and it should be used rather than using mysql parser directly.
Added GlobalParser variable to expose Doltgres parser for parsing view definition for now. It can also be used in places that needs doltgres-specific syntax parsing.

@jennifersp jennifersp changed the title get most of parsing into engine add parser interface in engine May 2, 2024
@jennifersp jennifersp marked this pull request as ready for review May 6, 2024 16:33
@jennifersp jennifersp requested a review from zachmu May 6, 2024 16:33
Copy link
Member

@zachmu zachmu left a comment

Choose a reason for hiding this comment

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

Parser interface looks good, putting it on the context looks bad.

It can go on the catalog if it needs to, but it probably shouldn't. The main place that needs it is the plan builder, it should be a field there.

@jennifersp jennifersp requested a review from zachmu May 6, 2024 19:33
Copy link
Member

@zachmu zachmu left a comment

Choose a reason for hiding this comment

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

Mostly looks good, just a couple pieces of feedback

sql/planbuilder/builder.go Outdated Show resolved Hide resolved
enginetest/server_engine.go Outdated Show resolved Hide resolved
engine.go Outdated Show resolved Hide resolved
@jennifersp jennifersp closed this May 7, 2024
@jennifersp jennifersp reopened this May 8, 2024
@jennifersp jennifersp merged commit 3278929 into main May 9, 2024
7 checks passed
@jennifersp jennifersp deleted the jennifer/parser branch May 9, 2024 16:43
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.

None yet

2 participants