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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 1 commit into from
Jun 3, 2024
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
82 changes: 77 additions & 5 deletions ...drools-drl-parser-tests/src/test/java/org/drools/drl/parser/antlr4/MiscDRLParserTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2174,12 +2174,44 @@ public void parse_QualifiedClassname() throws Exception {
}

@Test
public void parse_Accumulate() throws Exception {
final PackageDescr pkg = parseAndGetPackageDescrFromFile(
"accumulate.drl" );
void accumulate() {
final String drl = "rule R\n" +
"when\n" +
" accumulate( Person( $age : age );\n" +
" $avg : average( $age ) );\n" +
"then\n" +
"end";
RuleDescr rule = parseAndGetFirstRuleDescr(drl);

PatternDescr out = (PatternDescr) rule.getLhs().getDescrs().get( 0 );
assertThat(out.getObjectType()).isEqualTo("Object");
AccumulateDescr accum = (AccumulateDescr) out.getSource();
assertThat(accum.isExternalFunction()).isTrue();

List<AccumulateDescr.AccumulateFunctionCallDescr> functions = accum.getFunctions();
assertThat(functions.size()).isEqualTo(1);
assertThat(functions.get(0).getFunction()).isEqualTo("average");
assertThat(functions.get(0).getBind()).isEqualTo("$avg");
assertThat(functions.get(0).getParams()[0]).isEqualTo("$age");

final PatternDescr pattern = accum.getInputPattern();
assertThat(pattern.getObjectType()).isEqualTo("Person");

// accum.getInput() is always AndDescr
assertThat(accum.getInput()).isInstanceOfSatisfying(AndDescr.class, and -> {
assertThat(and.getDescrs()).hasSize(1);
assertThat(and.getDescrs().get(0)).isInstanceOfSatisfying(PatternDescr.class, patternDescr -> {
assertThat(patternDescr.getObjectType()).isEqualTo("Person");
});
});
}

@Test
void fromAccumulate() {
final PackageDescr pkg = parseAndGetPackageDescrFromFile("from_accumulate.drl" );

assertThat(pkg.getRules().size()).isEqualTo(1);
final RuleDescr rule = (RuleDescr) pkg.getRules().get( 0 );
final RuleDescr rule = pkg.getRules().get( 0 );
assertThat(rule.getLhs().getDescrs().size()).isEqualTo(1);

final PatternDescr outPattern = (PatternDescr) rule.getLhs().getDescrs().get( 0 );
Expand All @@ -2191,8 +2223,16 @@ public void parse_Accumulate() throws Exception {

assertThat(accum.isExternalFunction()).isFalse();

final PatternDescr pattern = (PatternDescr) accum.getInputPattern();
final PatternDescr pattern = accum.getInputPattern();
assertThat(pattern.getObjectType()).isEqualTo("Person");

// accum.getInput() is always AndDescr
assertThat(accum.getInput()).isInstanceOfSatisfying(AndDescr.class, and -> {
assertThat(and.getDescrs()).hasSize(1);
assertThat(and.getDescrs().get(0)).isInstanceOfSatisfying(PatternDescr.class, patternDescr -> {
assertThat(patternDescr.getObjectType()).isEqualTo("Person");
});
});
}

@Test
Expand Down Expand Up @@ -5232,4 +5272,36 @@ void durationChunk() {
// At the moment, the parser accepts any input and let the compile phase validate it.
assertThat(rule.getAttributes().get("duration").getValue()).isEqualTo("wrong input");
}

@Test
void accumulateWithEmptyActionAndReverse() {
final String drl = "rule R when\n" +
" Number() from accumulate( Number(),\n" +
" init( double total = 0; ),\n" +
" action( ),\n" +
" reverse( ),\n" +
" result( new Double( total ) )\n" +
" )\n" +
"then end";
RuleDescr rule = parseAndGetFirstRuleDescr(drl);

final PatternDescr outPattern = (PatternDescr) rule.getLhs().getDescrs().get( 0 );
final AccumulateDescr accum = (AccumulateDescr) outPattern.getSource();
assertThat(accum.getInitCode()).isEqualTo( "double total = 0;");
assertThat(accum.getActionCode()).isEmpty();
assertThat(accum.getReverseCode()).isEmpty();
assertThat(accum.getResultCode()).isEqualTo( "new Double( total )");

assertThat(accum.isExternalFunction()).isFalse();

final PatternDescr pattern = accum.getInputPattern();
assertThat(pattern.getObjectType()).isEqualTo("Number");

assertThat(accum.getInput()).isInstanceOfSatisfying(AndDescr.class, and -> {
assertThat(and.getDescrs()).hasSize(1);
assertThat(and.getDescrs().get(0)).isInstanceOfSatisfying(PatternDescr.class, patternDescr -> {
assertThat(patternDescr.getObjectType()).isEqualTo("Number");
});
});
}
}
0 ...g/drools/drl/parser/antlr4/accumulate.drl → ...ols/drl/parser/antlr4/from_accumulate.drl
View file
Open in desktop
File renamed without changes.
3 changes: 2 additions & 1 deletion drools-drl/drools-drl-parser/src/main/antlr4/org/drools/drl/parser/antlr4/DRLParser.g4
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Comment on lines -343 to +344
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

| accumulateFunction
)
RPAREN (SEMI)?
Expand Down
18 changes: 16 additions & 2 deletions drools-drl/drools-drl-parser/src/main/java/org/drools/drl/parser/antlr4/DRLVisitorImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -699,7 +699,8 @@ public PatternDescr visitLhsAccumulate(DRLParser.LhsAccumulateContext ctx) {
AccumulateDescr accumulateDescr = BaseDescrFactory.builder(new AccumulateDescr())
.withParserRuleContext(ctx)
.build();
accumulateDescr.setInput(visitLhsAndDef(ctx.lhsAndDef()));
// accumulateDescr.input is always AndDescr
accumulateDescr.setInput(wrapWithAndDescr(visitLhsAndDef(ctx.lhsAndDef()), ctx.lhsAndDef()));

// accumulate function
for (DRLParser.AccumulateFunctionContext accumulateFunctionContext : ctx.accumulateFunction()) {
Expand All @@ -714,6 +715,18 @@ public PatternDescr visitLhsAccumulate(DRLParser.LhsAccumulateContext ctx) {
return patternDescr;
}

private AndDescr wrapWithAndDescr(BaseDescr baseDescr, ParserRuleContext ctx) {
if (baseDescr instanceof AndDescr andDescr) {
return andDescr;
} else {
AndDescr andDescr = BaseDescrFactory.builder(new AndDescr())
.withParserRuleContext(ctx)
.build();
andDescr.addDescr(baseDescr);
return andDescr;
}
}

@Override
public Object visitLhsGroupBy(DRLParser.LhsGroupByContext ctx) {
GroupByDescr groupByDescr = BaseDescrFactory.builder(new GroupByDescr())
Expand Down Expand Up @@ -775,7 +788,8 @@ public AccumulateDescr visitFromAccumulate(DRLParser.FromAccumulateContext ctx)
AccumulateDescr accumulateDescr = BaseDescrFactory.builder(new AccumulateDescr())
.withParserRuleContext(ctx)
.build();
accumulateDescr.setInput(visitLhsAndDef(ctx.lhsAndDef()));
// accumulateDescr.input is always AndDescr
accumulateDescr.setInput(wrapWithAndDescr(visitLhsAndDef(ctx.lhsAndDef()), ctx.lhsAndDef()));
if (ctx.DRL_INIT() != null) {
// inline custom accumulate
accumulateDescr.setInitCode(getTextPreservingWhitespace(ctx.initBlockStatements));
Expand Down
Loading