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

[incubator-kie-drools-5948] [new-parser] Broken testIncompatibleListO… #5975

Conversation

tkobayas
Copy link
Contributor

Comment on lines -343 to +344
( DRL_INIT LPAREN initBlockStatements=chunk? RPAREN COMMA? DRL_ACTION LPAREN actionBlockStatements=chunk? RPAREN COMMA? ( DRL_REVERSE LPAREN reverseBlockStatements=chunk? RPAREN COMMA?)? DRL_RESULT LPAREN resultBlockStatements=chunk RPAREN
( DRL_INIT LPAREN initBlockStatements=chunk? RPAREN COMMA? DRL_ACTION LPAREN actionBlockStatements=chunk? RPAREN COMMA? DRL_REVERSE LPAREN reverseBlockStatements=chunk? RPAREN COMMA? DRL_RESULT LPAREN resultBlockStatements=chunk RPAREN
| DRL_INIT LPAREN initBlockStatements=chunk? RPAREN COMMA? DRL_ACTION LPAREN actionBlockStatements=chunk? RPAREN COMMA? DRL_RESULT LPAREN resultBlockStatements=chunk RPAREN
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not an elegant change. But in the previous rule, chunk? for the DRL_ACTION greedily eats DRL_RESULT result. So I changed to this one, which makes DRL_RESULT higher priority. If there is a better way to write, please let me know.

Copy link
Contributor

Choose a reason for hiding this comment

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

FYI, I've come up with a chunk rule version that can do parenthesis matching and nesting and is not greedy:

chunk : .*? (LPAREN chunk RPAREN .*?)* ;

If you want to experiment, change the chunk rule like that and revert lines 343-344. BUT there seems to be a serious performance problem with this approach as manifested by org.drools.testcoverage.regression.FusionAfterBeforeTest#testExpireEventsWhenSharingAllRules that times out with this chunk version.

I was curious if it's possible to make the chunk rule behave the same as the nesting chunk method in the old parser. It's nice that it can be done but it seems unusable for larger inputs. It's a reminder that we should be careful about using nongreedy parser subrules as also suggested in the docs:

Nongreedy subrules should be used sparingly because they complicate the recognition problem and sometimes make it tricky to decipher how the lexer will match text.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, @yurloc . I confirmed what you wrote. So the current fix would be this PR as is, and then we will consider "expected value format" rather than chunk, so that we can reduce the ambiguity, right? I listed it in #5972

@tkobayas
Copy link
Contributor Author

@yurloc @mariofusco @gitgabrio Please review, thanks!

@mariofusco mariofusco merged commit 32c2bda into apache:dev-new-parser Jun 3, 2024
0 of 4 checks passed
tkobayas added a commit to tkobayas/drools that referenced this pull request Jun 11, 2024
tkobayas added a commit to tkobayas/drools that referenced this pull request Oct 2, 2024
tkobayas added a commit to tkobayas/drools that referenced this pull request Oct 11, 2024
tkobayas added a commit that referenced this pull request Oct 11, 2024
rgdoliveira pushed a commit to rgdoliveira/drools that referenced this pull request Oct 24, 2024
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