feat: Improve null handling in filter expressions #685
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
korthout
force-pushed
the
mob-582-filter-null
branch
2 times, most recently
from
a22cd23
to
ea26b0c
Compare
|
@saig0 I've tried to describe what we did. Please have a look 🙇 |
saig0
approved these changes
Jul 21, 2023
src/test/scala/org/camunda/feel/impl/interpreter/InterpreterListExpressionTest.scala
Outdated
Show resolved
Hide resolved
korthout
force-pushed
the
mob-582-filter-null
branch
from
24feb5f
to
1ab352d
Compare
Co-authored-by: berkaycanbc <berkay.can@camunda.com> Co-authored-by: Nicola Puppa <nicola.puppa@camunda.com> Co-authored-by: Nikola Koevski <nikola.koevski@camunda.com> Co-authored-by: Remco Westerhoud <remco@westerhoud.nl>
This allows us to use `null` in filters where `null` is compared to the item, or when the item is `null` explicitly. It does not yet help us deal with variables that are missing, as this produces an error. Co-authored-by: berkaycanbc <berkay.can@camunda.com> Co-authored-by: Nicola Puppa <nicola.puppa@camunda.com> Co-authored-by: Remco Westerhoud <remco@westerhoud.nl> Conflicts: src/main/scala/org/camunda/feel/impl/interpreter/FeelInterpreter.scala Resolution: we ignore the change on the other case (previously a ValError, now error). Instead, we use the change we proposed (ignore this entry and continue with the others).
Since we don't yet support list expressions with missing variables as `null` values in that list, there is no reason to work on the filter expressions for those list expressions. Example, [1,2,x,4] should eventually evaluate to [1,2,null,4] but at this time we evaluate it to an error stating that the variable is missing. Testing that [1,2,x,4][item > 3] works correctly is therefore not yet possible. Co-authored-by: berkaycanbc <berkay.can@camunda.com> Co-authored-by: Nicola Puppa <nicola.puppa@camunda.com> Co-authored-by: Remco Westerhoud <remco@westerhoud.nl>
When the filter doesn't (always) return a boolean or a number (to filter by index), then we no longer want to fail. Instead, we want to provide a list of all the elements that did match the filter expression. So, when the filter does not even evaluate to a boolean value, then we want to product an empty list. And, if the filter sometimes evaluates to a boolean value and sometimes doesn't, then we want a list of those values for which the filter did evaluate to the boolean `true`. As both these cases are very similar, it makes sense to combine them into a single test. Co-authored-by: berkaycanbc <berkay.can@camunda.com> Co-authored-by: Nicola Puppa <nicola.puppa@camunda.com> Co-authored-by: Remco Westerhoud <remco@westerhoud.nl>
These cases are actually quite different from each other. We can highlight this by splitting them off into separate tests.
korthout
force-pushed
the
mob-582-filter-null
branch
from
1ab352d
to
96aa1e9
Compare
|
@korthout super. Thank you. 🚀 |
saig0
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This changes the behavior of filter expressions to be more lenient.
Filter expressions now filter the list even when the filter expression is not evaluating to a boolean (for item comparison) or a number (for direct access).
When the filter expression would previously cause an error, the error is now suppressed and a list with only those values that match the filter expression is returned.
Please note that this pull request does not fully resolve issue #582. It does not yet deal with lists of contexts where the filter expression refers to missing properties of those contexts. This should be resolved in another pull request.
Related issues
partially deals with #582 (filters over context lists are not yet covered)