fix: better null handling for unary tests involving special character ? #882
Conversation
|
|
|
@spearsandshields thank you for your contribution. 🎉 I'll review your PR this week. 👀 |
spearsandshields
force-pushed
the
bugfix
branch
from
b83fb10
to
3bdf557
Compare
|
@saig0 , thanks. |
There was a problem hiding this comment.
@spearsandshields the solution solves the concrete issue. 👍 However, I don't see a solution for the general issue with the unary-tests (see the examples in the comment).
I would aim for a general solution that reflects the description of the DMN specification (i.e. an expression with ? must evaluate to true).
At the moment, I can't share a concrete design. But, this is not the way, sorry.
| expression match { | ||
| case comparison: Comparison => | ||
| if (comparison.x == ConstInputValue || comparison.y == ConstInputValue) { | ||
| x match { | ||
| case ErroredVal(value, _) => return value | ||
| case _ => // Continue | ||
| } | ||
| } | ||
| case _ => // Continue if not a Comparison or ConstInputValue not found | ||
| } |
There was a problem hiding this comment.
❌ This modification solves the case for the comparison (? > null). However, it doesn't solve the issue in general.
The following expressions still return true instead of null:
// for ? = null
? > 20 and ? < 40
// output: true
? > 20 or ? < 40
// output: true
?
// output: true
| val result = evalInternal(expression) | ||
| result match { | ||
| case ErroredVal(value, _) => | ||
| value | ||
| case _ => result | ||
| } |
There was a problem hiding this comment.
🔧 Having two different evaluation methods with the same signature makes the code harder to understand and could lead to issues.
| case class ErroredVal(value: Val, errorType: EvaluationFailureType) extends Val { | ||
| override def toString: String = value.toString | ||
| } |
There was a problem hiding this comment.
❌ The new type is only used in combination with ValNull. It seems like the failure type should be an additional property of ValNull. And, it is used only within the interpreter.
|
Close this PR in favor of #887. @spearsandshields thank you for your attempt. 🙇 |
|
Thanks for drafting this more thorough fix. @saig0 |
@spearsandshields I will build new patch releases of the FEEL engine soon. So, the fix will be included in the upcoming Camunda patch releases. 🏗️ |
|
@saig0 , thanks and this is huge! Is there a rough timeline where I can expect the patch release? |
|
@spearsandshields you can expect the next patch release in the next few weeks. 🚧 |
Description
This PR is drafted to address null value handling in unary test, especially when special charactor ? is involved.
Gist of the fix:
Whenever something evaluates to null, due to errors. Such as not-comaparable data types, we need a way to embed/box that information, and pass it along to the callers. So the callers can determine how they want to handle these special values.
Therefore, I created a artifact valueType called ErroredVal, this is different from ValError, as ValError results in not-resumable error. ErroredValue allow the logic to continue, and callers can decide when/how to handle it at their choosing.
Adding this mechanism allow me to swap some ValNull to boxed ErroredVal.
After this, we need to ensure the ErroredVal does not affect the final result. In order to achieve that, I moved logic from eval to evalInternal, and made eval function behave like a wrapper than returns the unboxed ErroredVal. (this allow tests to pass as normal)
With the additional of this new mechanism, I made some changes to unaryTestExpression:
Related issues
#864
closes #