You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@kie.apache.org by "tkobayas (via GitHub)" <gi...@apache.org> on 2024/05/27 09:23:33 UTC
[PR] [incubator-kie-drools-5948] [new-parser] Broken testIncompatibleListO… [incubator-kie-drools]
tkobayas opened a new pull request, #5975:
URL: https://github.com/apache/incubator-kie-drools/pull/5975
…nAccumulateFunction
**Issue**:
* https://github.com/apache/incubator-kie-drools/issues/5948
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] [incubator-kie-drools-5948] [new-parser] Broken testIncompatibleListO… [incubator-kie-drools]
Posted by "tkobayas (via GitHub)" <gi...@apache.org>.
tkobayas commented on code in PR #5975:
URL: https://github.com/apache/incubator-kie-drools/pull/5975#discussion_r1620176032
##########
drools-drl/drools-drl-parser/src/main/antlr4/org/drools/drl/parser/antlr4/DRLParser.g4:
##########
@@ -340,7 +340,8 @@ fromAccumulate := ACCUMULATE LEFT_PAREN lhsAnd (COMMA|SEMICOLON)
) RIGHT_PAREN
*/
fromAccumulate : (DRL_ACCUMULATE|DRL_ACC) LPAREN lhsAndDef (COMMA|SEMI)
- ( 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
Review Comment:
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 https://github.com/apache/incubator-kie-drools/issues/5972
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] [incubator-kie-drools-5948] [new-parser] Broken testIncompatibleListO… [incubator-kie-drools]
Posted by "mariofusco (via GitHub)" <gi...@apache.org>.
mariofusco merged PR #5975:
URL: https://github.com/apache/incubator-kie-drools/pull/5975
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] [incubator-kie-drools-5948] [new-parser] Broken testIncompatibleListO… [incubator-kie-drools]
Posted by "yurloc (via GitHub)" <gi...@apache.org>.
yurloc commented on code in PR #5975:
URL: https://github.com/apache/incubator-kie-drools/pull/5975#discussion_r1615897263
##########
drools-drl/drools-drl-parser/src/main/antlr4/org/drools/drl/parser/antlr4/DRLParser.g4:
##########
@@ -340,7 +340,8 @@ fromAccumulate := ACCUMULATE LEFT_PAREN lhsAnd (COMMA|SEMICOLON)
) RIGHT_PAREN
*/
fromAccumulate : (DRL_ACCUMULATE|DRL_ACC) LPAREN lhsAndDef (COMMA|SEMI)
- ( 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
Review Comment:
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](https://github.com/antlr/antlr4/blob/master/doc/wildcard.md#nongreedy-parser-subrules):
> 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] [incubator-kie-drools-5948] [new-parser] Broken testIncompatibleListO… [incubator-kie-drools]
Posted by "tkobayas (via GitHub)" <gi...@apache.org>.
tkobayas commented on code in PR #5975:
URL: https://github.com/apache/incubator-kie-drools/pull/5975#discussion_r1615774192
##########
drools-drl/drools-drl-parser/src/main/antlr4/org/drools/drl/parser/antlr4/DRLParser.g4:
##########
@@ -340,7 +340,8 @@ fromAccumulate := ACCUMULATE LEFT_PAREN lhsAnd (COMMA|SEMICOLON)
) RIGHT_PAREN
*/
fromAccumulate : (DRL_ACCUMULATE|DRL_ACC) LPAREN lhsAndDef (COMMA|SEMI)
- ( 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
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] [incubator-kie-drools-5948] [new-parser] Broken testIncompatibleListO… [incubator-kie-drools]
Posted by "tkobayas (via GitHub)" <gi...@apache.org>.
tkobayas commented on PR #5975:
URL: https://github.com/apache/incubator-kie-drools/pull/5975#issuecomment-2138896795
@yurloc @mariofusco @gitgabrio Please review, thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org