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

Drop plan parser dependency on antlr4 #1201

Open
wants to merge 4 commits into
base: refactor-parsing-pipeline
Choose a base branch
from

Conversation

ericvergnaud
Copy link
Contributor

Our current PlanParser enforces use of ANTLR4 to parse input.
This dependency is undesirable, and does not provide any benefit.

Thanks to #1200, this is no longer required.

This PR:
Removes dependency on antlr4 in PlanParser

Requires #1200

Progresses #869

Supersedes #1192

Copy link

github-actions bot commented Nov 13, 2024

Coverage tests results

448 tests  ±0   414 ✅ ±0   4s ⏱️ ±0s
  6 suites ±0    34 💤 ±0 
  6 files   ±0     0 ❌ ±0 

Results for commit e857f0d. ± Comparison against base commit c8b5af8.

♻️ This comment has been updated with latest results.

@ericvergnaud ericvergnaud marked this pull request as ready for review November 14, 2024 08:32
@nfx nfx requested a review from jimidle November 14, 2024 14:43
Copy link
Contributor

@jimidle jimidle left a comment

Choose a reason for hiding this comment

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

Making this less generic will force reimplementation of code in the specific plan parsers. I don't think that this is a good idea. Dependency on ANTLR is not really a problem for the parser I think.

Perhaps you can explain why you want to dothis?

@ericvergnaud
Copy link
Contributor Author

Making this less generic will force reimplementation of code in the specific plan parsers. I don't think that this is a good idea. Dependency on ANTLR is not really a problem for the parser I think.

Perhaps you can explain why you want to do this?

I agree it's not a big problem (yet) however:

  • dropping it is not a big problem either
  • I foresee various situations where we want to parse a plan using an existing parser that is not ANTLR based.

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.

2 participants