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

feat: Improve null handling in filter expressions #685

Merged
merged 5 commits into from
Jul 21, 2023
Merged

Conversation

korthout
Copy link
Member

@korthout korthout commented Jul 13, 2023

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)

@korthout korthout marked this pull request as ready for review July 20, 2023 12:02
@korthout korthout requested a review from saig0 as a code owner July 20, 2023 12:02
@korthout korthout force-pushed the mob-582-filter-null branch 2 times, most recently from a22cd23 to ea26b0c Compare July 20, 2023 12:12
@korthout
Copy link
Member Author

@saig0 I've tried to describe what we did. Please have a look 🙇

Copy link
Member

@saig0 saig0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@korthout LTGM. 🚀

I have one minor comment about the test case.

@korthout korthout force-pushed the mob-582-filter-null branch from 24feb5f to 1ab352d Compare July 21, 2023 12:09
korthout and others added 5 commits July 21, 2023 14:10
Co-authored-by: berkaycanbc <[email protected]>
Co-authored-by: Nicola Puppa <[email protected]>
Co-authored-by: Nikola Koevski <[email protected]>
Co-authored-by: Remco Westerhoud <[email protected]>
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 <[email protected]>
Co-authored-by: Nicola Puppa <[email protected]>
Co-authored-by: Remco Westerhoud <[email protected]>

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 <[email protected]>
Co-authored-by: Nicola Puppa <[email protected]>
Co-authored-by: Remco Westerhoud <[email protected]>
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 <[email protected]>
Co-authored-by: Nicola Puppa <[email protected]>
Co-authored-by: Remco Westerhoud <[email protected]>
These cases are actually quite different from each other. We can
highlight this by splitting them off into separate tests.
@korthout korthout force-pushed the mob-582-filter-null branch from 1ab352d to 96aa1e9 Compare July 21, 2023 12:14
@korthout
Copy link
Member Author

@saig0 Please have another look. I had to resolve a conflict in 04c123a.

@saig0
Copy link
Member

saig0 commented Jul 21, 2023

@korthout super. Thank you. 🚀

@saig0 saig0 merged commit 2fe6c5e into main Jul 21, 2023
@saig0 saig0 deleted the mob-582-filter-null branch July 21, 2023 13:01
@saig0 saig0 changed the title Improve null handling in filter expressions feat: Improve null handling in filter expressions Jul 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants