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

Experiment: New DRL parser #5678

Closed
tkobayas opened this issue Feb 8, 2024 · 14 comments
Closed

Experiment: New DRL parser #5678

tkobayas opened this issue Feb 8, 2024 · 14 comments
Assignees

Comments

@tkobayas
Copy link
Contributor

tkobayas commented Feb 8, 2024

Issue Description:

Migrate a new DRL parser based on antlr 4, because the current DRL parser (based on antlr 3) is hard-coded in the generated Java codes that make it hard to maintain.

Acceptance Criteria:

  • Possibly redundant token definitions (DRL_STRING_LITERAL in the DRL lexer and STRING_LITERAL in the Java lexer).
  • Inconsistent operator tokens (we now have DRL_MATCHES but not DRL_CONTAINS).
  • What's the benefit of the built-in operator token definitions if we still need to accept pluggable operators?
  • What to do with the overlap between the expression and DRL parsers?

Out of Scope:

  • Merging the new parser to main is out of scope of this issue. When it's ready, we will file a new issue for that
  • Once the parser is migrated to main, drools-lsp will remove its drools-parser and refer the new parser
@tkobayas
Copy link
Contributor Author

tkobayas commented Feb 8, 2024

Migrate a new DRL parser in the drools code base with a feature branch dev-new-parser.

This is done by #5677 #5682

@tkobayas
Copy link
Contributor Author

tkobayas commented Feb 8, 2024

  • Fix and improve the parser by solving the test failures
    • It could be several separated issues/PRs which we can work on in parallel

Each issue would be listed in this issue so that this issue (5678) is the parent issue.

@tkobayas
Copy link
Contributor Author

tkobayas commented Feb 8, 2024

Child issue:
Test failure : RHS end without preceding white-space
#5679

@tkobayas
Copy link
Contributor Author

tkobayas commented Feb 9, 2024

Child issue:
Test failure : Unknown language level
#5681

@tkobayas
Copy link
Contributor Author

tkobayas commented Feb 9, 2024

Child issue:
Better implementation switch
#5683

@tkobayas
Copy link
Contributor Author

tkobayas commented Jun 11, 2024

As of 2024-07-11, unit tests are all green. Remaining child issues are low priority, so they can be handled after merging to main.

#5799
#5807
#5856
#5874
#5938

The next step is to create a PR to merge this branch to main branch. Notes:

  • Merging has to be done after Drools 10.0 release.
  • Until the Drools 10.0 release, mark the PR [DO-NOT-MERGE] and make it draft.
  • Create 2 PRs. One makes the new parser not default parser. So it shouldn't affect the default behavior. It is the one to be merged. The other makes the new parser default parser (with a system property switch). It is in order to check any regression error. It's not to be merged, so draft PR.
  • After Drools 10 release, discuss the future plan in dev at kie.apache ML.

@anand188
Copy link

@tkobayas Hi is there any timeline when we can expect 10.0 with antlr4-runtime updated will be released ?

@tkobayas
Copy link
Contributor Author

@anand188 There is no timeline forecast yet. I hope 10.0 (without new parser) release will happen in 1 or 2 months, but not very sure. New parser will be included in the next minor version (probably 10.1).

@anand188
Copy link

@tkobayas is that fair statement we can expect 10.1 before December, is this something we can wait for ?

@tkobayas
Copy link
Contributor Author

tkobayas commented Aug 30, 2024

@anand188 Sorry, we cannot provide a timeline forecast at the moment.

Btw, I'm interested in why you wait for the antlr4 based parser. Could you let me know? It's implementation details and doesn't directly affect users.

@seyoatda
Copy link

seyoatda commented Sep 10, 2024

@anand188 Sorry, we cannot provide a timeline forecast at the moment.

Btw, I'm interested in why you wait for the antlr4 based parser. Could you let me know? It's implementation details and doesn't directly affect users.

Hi, I'm planning to write a drl formatter/prettier, since there isn't an official one, nor any 3rd-party one. (If there is, plz let me know so that I can just use it.)
I took a look at the DRL6Lexer.g & DRL6Expressions.g files, and it's a bit annoying to migrate them from antlr3 to anltr4.
Currently I'm handling the migration issues, but if the drools team have done with antlr4 lexer and parser definitions, it‘s very helpful XD. 😁

@tkobayas
Copy link
Contributor Author

Hi @seyoatda ,

It's great to hear that you are willing to develop a drl formatter/prettier. I haven't heard of a drl formatter/prettier. https://github.com/kiegroup/drools-lsp is aiming at a DRL editor, but the development is currently suspended until the new parser will be merged. Also it doesn't include a formatter/prettier feature yet.

it's a bit annoying to migrate them from antlr3 to anltr4.

The new parser is not yet merged, but you can use the files in my branch.

https://github.com/tkobayas/drools/tree/incubator-kie-drools-5988-merge/drools-drl/drools-drl-parser/src/main/antlr4/org/drools/drl/parser/antlr4

DRLLexer.g4, DRLParser.g4, DRL6Expressions.g4 are Apache License, Version 2.0.
JavaLexer.g4, JavaParser.g4 are BSD license. (from antlr/grammars-v4)

Cheers,

@tkobayas
Copy link
Contributor Author

The new parser has been merged into the main branch (not in 10.0.x branch). The new parser is not enabled by default at the moment. You can enable it by setting system property drools.drl.antlr4.parser.enabled to true.

Remaining lower priority issues are aggregated in this Github issue: #5988

@github-project-automation github-project-automation bot moved this from 📋 Backlog to 🎯 Done in 🦉 KIE Podling Board Oct 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 🎯 Done
Development

No branches or pull requests

4 participants