[DO-NOT-MERGE][incubator-kie-drools-6220] Slim down DRL syntax with New Antlr4 Parser #6225

tkobayas · 2025-01-21T09:20:38Z

Issue:

Slim down DRL syntax with New Antlr4 Parser #6220
Introduce DRL10
WIP

kie-ci3 · 2025-01-23T09:19:52Z

PR job #6 was: UNSTABLE
Possible explanation: This should be test failures

Reproducer

build-chain build full_downstream -f 'https://raw.githubusercontent.com/${AUTHOR:apache}/incubator-kie-kogito-pipelines/${BRANCH:main}/.ci/buildchain-config-pr-cdb.yaml' -o 'bc' -p apache/incubator-kie-drools -u #6225 --skipParallelCheckout

NOTE: To install the build-chain tool, please refer to https://github.com/kiegroup/github-action-build-chain#local-execution

Please look here: https://ci-builds.apache.org/job/KIE/job/drools/job/main/job/pullrequest_jobs/job/drools-pr/job/PR-6225/6/display/redirect

Test results:

PASSED: 15528
FAILED: 1

Those are the test failures:

PR check / Build projects / maven.invoker.it.drools-test-coverage-jars-with-invoker.drools-test-coverage-jars-with-invoker

The build exited with code 143. See /home/jenkins/workspace/llrequest_jobs_drools-pr_PR-6225/kogito-pipelines/bc/apache_incubator-kie-drools/drools-test-coverage/test-compiler-integration/../drools-test-coverage-jars/drools-test-coverage-jars-with-invoker/build.log for details.

- Introduce DRL10 - WIP

kie-ci3 · 2025-02-03T09:53:05Z

PR job #14 was: UNSTABLE
Possible explanation: This should be test failures

Reproducer

build-chain build full_downstream -f 'https://raw.githubusercontent.com/${AUTHOR:apache}/incubator-kie-kogito-pipelines/${BRANCH:main}/.ci/buildchain-config-pr-cdb.yaml' -o 'bc' -p apache/incubator-kie-drools -u #6225 --skipParallelCheckout

NOTE: To install the build-chain tool, please refer to https://github.com/kiegroup/github-action-build-chain#local-execution

Please look here: https://ci-builds.apache.org/job/KIE/job/drools/job/main/job/pullrequest_jobs/job/drools-pr/job/PR-6225/14/display/redirect

Test results:

PASSED: 14920
FAILED: 3

Those are the test failures:

org.drools.model.codegen.execmodel.generator.drlxparse.ConstraintParserTest.testImplicitCastExpressionWithOr

Cannot invoke "org.drools.drl.ast.descr.RuleDescr.getName()" because "this.ruleDescr" is null

org.drools.model.codegen.execmodel.generator.drlxparse.ConstraintParserTest.testNullSafeExpressionsWithOr

Cannot invoke "org.drools.drl.ast.descr.RuleDescr.getName()" because "this.ruleDescr" is null

org.kie.maven.plugin.it.kie-maven-plugin-test-kjar-15-yaml.kie-maven-plugin-test-kjar-15-yaml

The build exited with code 143. See /home/jenkins/workspace/llrequest_jobs_drools-pr_PR-6225/kogito-pipelines/bc/apache_incubator-kie-drools/kie-maven-plugin/target/it/kie-maven-plugin-test-kjar-15-yaml/build.log for details.

tkobayas · 2025-02-05T04:36:04Z

After review, revert GHA changes. So this is still "Draft"

- Disable drools-verifier test for DRL10

- adjust unit tests

tkobayas · 2025-02-05T09:01:12Z

.github/workflows/pr-drools.yml

@@ -61,7 +61,7 @@ jobs:
      - name: Build Chain
        uses: apache/incubator-kie-kogito-pipelines/.ci/actions/build-chain@main
        env:
-          BUILD_MVN_OPTS_CURRENT: '-Dfull -Dreproducible'
+          BUILD_MVN_OPTS_CURRENT: '-Dfull -Dreproducible -DenableNewParser'


This is a temporal change to test the new parser (= DRL10). I'll revert this after review.

tkobayas · 2025-02-05T09:15:04Z

drools-drl/drools-drl-parser/src/main/java/org/drools/drl/parser/lang/ParserHelper.java

+    private static int logCounterHalfConstraint = 0;
+    private static int logCounterCustomOperator = 0;
+    private static int logCounterInfixAnd = 0;
+    private static int logCounterInfixOr = 0;
+    private static int logCounterAnnotationInLhsPattern = 0;
+    private static int logCounterAgendaGroup = 0;


A counter to suppress log flooding. Using static int is a bit rough way, but ParserHelper instance is short-living, so we cannot use instance fields. Managing the counter in an external class seems to be too much for logging.

tkobayas · 2025-02-05T09:20:49Z

drools-drl/drools-drl-parser/src/main/java/org/drools/drl/parser/lang/ParserHelper.java

+    public static void logHalfConstraintWarn(String message) {
+        if (logCounterHalfConstraint > 10) {
+            return; // suppress further warnings
+        }
+
+        logCounterHalfConstraint++;
+        LOG.warn(message);
+        if (logCounterHalfConstraint == 10) {
+            LOG.warn("Further warnings about half constraints will be suppressed.");
+        }
+    }


The counter usage is repetitive in the log methods. I may be able to refactor them, but I guess it cannot be improved much. If you want me to refactor, I can try and share.

tkobayas · 2025-02-05T09:23:45Z

drools-drl/drools-drl-parser/src/main/java/org/drools/drl/parser/lang/DRL6Parser.java

@@ -1558,6 +1558,7 @@ public AttributeDescr attribute(AttributeSupportBuilder<?> as) {
                            DroolsSoftKeywords.GROUP)) {
                attribute = stringAttribute(as,
                        new String[]{DroolsSoftKeywords.AGENDA, "-", DroolsSoftKeywords.GROUP});
+                helper.logAgendaGroupWarn(attribute);


Note that DRL6Parser.java was generated from .g file in the past, and then has been maintained by hard-coding in the java file. So I add the logging directly, not modifying .g file.

tkobayas · 2025-02-05T09:30:42Z

drools-legacy-test-util/src/test/java/org/drools/mvel/CommonTestMethodBase.java

+    public static String replaceAgendaGroupIfRequired(String drl) {
+        if (DrlParser.ANTLR4_PARSER_ENABLED) {
+            // new parser (DRL10) supports only ruleflow-group, dropping agenda-group
+            return drl.replaceAll("agenda-group", "ruleflow-group");
+        }
+        return drl;
+    }


replaceAgendaGroupIfRequired is defined in 3 places in this PR, because we don't have a common test util module which can access drools-drl-parser.

kie-test-util doesn't depend on any drools modules

drools-legacy-test-util is meant be 'legacy' (to be removed)

I think this topic should be separated from this PR and I will consult @pibizza .

tkobayas · 2025-02-05T09:42:20Z

drools-test-coverage/test-suite/src/test/java/org/drools/testcoverage/common/util/KieUtil.java

            fileSystem.write(resource);
        }
        fileSystem.writeKModuleXML(kieModuleModel.toXML());
        return fileSystem;
    }

+    private static Resource newResourceForNewParserIfRequired(Resource resource) {


This method is used for the new parser testing (not enabled by default). Actually, only to convert agenda-group to ruleflow-group because agenda-group is not allowed in DRL10, but it's widely used in the test cases (cannot manage with @DisabledIfSystemProperty). When we drop DRL6 in the future, we will be able to fully replace agenda-group in test cases and remove this conversion.

tkobayas · 2025-02-05T09:46:55Z

drools-verifier/drools-verifier-drl/pom.xml

+            <artifactId>maven-surefire-plugin</artifactId>
+            <configuration>
+              <!-- drools-verifier doesn't support DRL10 -->
+              <skipTests>true</skipTests>


DRLs in drools-verifier hit the dropped syntax in DRL10 (e.g. half constraint). I can fix them, but probably we want to discuss whether will keep maintaining drools-verifier or not. So I delay the fix for now.

tkobayas · 2025-02-05T09:58:59Z

Hi @mariofusco @porcelli @gitgabrio @baldimir @mdproctor @pibizza ,

Here is the DRL10 syntax slim down.

As written in https://docs.google.com/document/d/1Ibmj-koAMbeaungHuFeQtw2zD03YJcNJkJ-kdN8ugks/edit?tab=t.0 ,

This PR does:

Introduce Language Level DRL10 (default is still DRL6)
Changes in DRL10 are
- Drop half constraint
- Custom operator requires a prefix ##
- Drop || / && as alternatives to infix or / and (|| / && in constraint remain supported)
- Drop annotation inside LHS Pattern (annotation in other places e.g. @watch remain supported)
- Consolidate agenda-group to ruleflow-group
Log warnings when the deprecated syntax is used in DRL6
Log warnings when the deprecated Language Level DRL5 and DRL6_STRICT is used

The DRL10 introduction plan is #6221

Please review, thanks!

tkobayas added the do-not-merge label Jan 21, 2025

tkobayas force-pushed the incubator-kie-drools-6220-slimdown branch 3 times, most recently from 9fcc046 to 76f8c98 Compare January 23, 2025 08:36

tkobayas force-pushed the incubator-kie-drools-6220-slimdown branch from 1457c6f to e59ed36 Compare January 24, 2025 03:35

[incubator-kie-drools-6220] Slim down DRL syntax with New Antlr4 Parser

4b03019

- Introduce DRL10 - WIP

tkobayas force-pushed the incubator-kie-drools-6220-slimdown branch 2 times, most recently from fcf232c to 37a218d Compare January 30, 2025 09:09

tkobayas mentioned this pull request Jan 31, 2025

[DO-NOT-MERGE][WIP][incubator-kie-drools-6220] testing default DRL6 #6240

Draft

tkobayas force-pushed the incubator-kie-drools-6220-slimdown branch 2 times, most recently from 2559994 to 428a1e6 Compare February 3, 2025 09:51

tkobayas added 9 commits February 5, 2025 14:43

- a prefix for custom operator

d76bdd4

- drop half constraint

6079976

- Disable drools-verifier test for DRL10

- drop infix doubleAmpersand and doublePipe

ef0a455

- drop Annotation inside LHS

7be57d5

- additional test for end token

5a28d5d

- replace agenda-group with ruleflow-group

e0872ff

- adjust unit tests

- Add warnings for deprecated DRL syntax when used in DRL6

2a95d6c

- release notes and docs

30ecdd1

- gha test DRL10

b7c1b54

tkobayas force-pushed the incubator-kie-drools-6220-slimdown branch from 8a9bbff to b7c1b54 Compare February 5, 2025 05:54

tkobayas commented Feb 5, 2025

View reviewed changes

tkobayas changed the title ~~[DO-NOT-MERGE][WIP][incubator-kie-drools-6220] Slim down DRL syntax with New Antlr4 Parser~~ [DO-NOT-MERGE][incubator-kie-drools-6220] Slim down DRL syntax with New Antlr4 Parser Feb 5, 2025

tkobayas requested review from porcelli, baldimir, mariofusco, mdproctor, gitgabrio and pibizza February 6, 2025 03:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DO-NOT-MERGE][incubator-kie-drools-6220] Slim down DRL syntax with New Antlr4 Parser #6225

[DO-NOT-MERGE][incubator-kie-drools-6220] Slim down DRL syntax with New Antlr4 Parser #6225

tkobayas commented Jan 21, 2025

kie-ci3 commented Jan 23, 2025

kie-ci3 commented Feb 3, 2025

tkobayas commented Feb 5, 2025 •

edited

Loading

tkobayas Feb 5, 2025

tkobayas Feb 5, 2025

tkobayas Feb 5, 2025

tkobayas Feb 5, 2025

tkobayas Feb 5, 2025

tkobayas Feb 5, 2025

tkobayas Feb 5, 2025

tkobayas commented Feb 5, 2025

[DO-NOT-MERGE][incubator-kie-drools-6220] Slim down DRL syntax with New Antlr4 Parser #6225

Are you sure you want to change the base?

[DO-NOT-MERGE][incubator-kie-drools-6220] Slim down DRL syntax with New Antlr4 Parser #6225

Conversation

tkobayas commented Jan 21, 2025

kie-ci3 commented Jan 23, 2025

kie-ci3 commented Feb 3, 2025

tkobayas commented Feb 5, 2025 • edited Loading

tkobayas Feb 5, 2025

Choose a reason for hiding this comment

tkobayas Feb 5, 2025

Choose a reason for hiding this comment

tkobayas Feb 5, 2025

Choose a reason for hiding this comment

tkobayas Feb 5, 2025

Choose a reason for hiding this comment

tkobayas Feb 5, 2025

Choose a reason for hiding this comment

tkobayas Feb 5, 2025

Choose a reason for hiding this comment

tkobayas Feb 5, 2025

Choose a reason for hiding this comment

tkobayas commented Feb 5, 2025

tkobayas commented Feb 5, 2025 •

edited

Loading