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

Don't fail a range comparison in FEEL when a unary test range is used #1744

Closed
bncriju opened this issue Jan 10, 2025 · 3 comments · Fixed by apache/incubator-kie-drools#6231
Assignees
Labels
area:dmn Related to DMN area:engine Related to the runtime engines type:bug Something is behaving unexpectedly

Comments

@bncriju
Copy link

bncriju commented Jan 10, 2025

There is a DMN TCK failure, when running with Drools:

[ERROR] 0068-feel-equality-test-01.range_010 – Time elapsed: 0.016 s <<< ERROR!
java.lang.RuntimeException: ERROR: FAILURE: 'range_010' expected='false' but found='null'

The test is located here (1). Based on the error, it looks like the engine fails the evaluation on an exception somewhere. It should behave based on the definition in the test and return false.

Options how to debug:

Replicating the test case directly in Drools repository, so it gets a proper stack trace pointing to the code path being executed, to be able to see the code areas, which may need a fix. If the test case is mainly just a FEEL expression (FEEL is a language used to write expressions in DMN), the FEEL expression could be added to a test class, e.g. this one (2), so it can be run in isolation.

(1) https://github.com/dmn-tck/tck/blob/master/TestCases/compliance-level-3/0068-feel-equality/0068-feel-equality.dmn#L1311

(2) https://github.com/apache/incubator-kie-drools/blob/16897949cb6b691ec6b919cd4593288ebbd00cee/kie-dmn/kie-dmn-feel/src/test/java/org/kie/dmn/feel/runtime/FEELRangesTest.java#L36

@yesamer yesamer added type:bug Something is behaving unexpectedly area:dmn Related to DMN area:engine Related to the runtime engines labels Jan 10, 2025
@yesamer
Copy link

yesamer commented Jan 10, 2025

range_010
range_011
range_012

Those 3 tests are failing in 0068 suite test.

@gitgabrio
Copy link

gitgabrio commented Jan 21, 2025

After deeper investigation, it turns out that there are different problems related to this ticket.

1
The TCK range tests (e.g. (&gt; 10) = (10..null) (=10) = [10..10]) seems to verify that the expression is different even if its evaluation is the same.
OUr code

  1. translate the (&gt; 10) = (10..null) to an InfixOpNode
  2. evaluate left and right part of the expression (inside EqExecutor)
  3. compares them

So, first of all I think there is an issue in the InfixOpNode/EqExecutor, that, in this case, should verify if the expression is the same, and not if its evaluation is the same.

(&gt; 10) = (10..null) -> FALSE succeed "by chance" because we introduced the undefined, so the range returned is (10..undefined) , but actually the behavior is wrong (replacing (10..null) with (10..undefined) demonstrates it)

2
(=10) = (=10) -> TRUE fails because (=10) is not translated to Range, and when the code enter BooleanEvalHelper.compare, there is not an "if " dealing with UnaryTestImpl, and returns null

Problem 2 could be solved implementing conversion of (=10) to [10..10] , but then problem 1 ((=10) = [10..10] -> FALSE )

would still be there because, during execution, left and right side would be evaluated to same object before comparison.

@baldimir @yesamer wdyt ?

@gitgabrio
Copy link

gitgabrio commented Jan 24, 2025

I made modification in my own branch, and I have fixed some use cases.
Anyway, a new issue arise, namely

Expression
= null
Test
context.set("in1", null);

Currently, the expression is converted to UnaryTestNode
Then, at execution, the utEqualSemantic method is invoked, that does not check if the right object is ´null´ (as in this case) so it returns true (because both left and right are null)

With my modification, = null is translated to Range, but then the createInUnaryTest method is invoked. In that case, if the input is null (as in the current test) the method immediately returns false, and the test fails.
Beside, if the input is different then null, then the RangeImpl.includes method is invoked, that returns null if the parameter is null.
So, on one side it is not clear if = null should be translated to [null .. null] or [undefined .. undefined]; on the other side, we should review the behavior of UnaryTestNode.createInUnaryTest when the input is null (I don't know why it is different then utEqualSemantic)

@baldimir @yesamer

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:dmn Related to DMN area:engine Related to the runtime engines type:bug Something is behaving unexpectedly
Projects
None yet
3 participants