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

Fix aggregator batching for nullable aggregates #59

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

folmate
Copy link

@folmate folmate commented May 16, 2024

Problem:
Aggregators such as min/max have a null initial value, resulting in changes not being propagated.
(The entry set of oldValues discards keys, where the value is null.)

Solution:
Initial values are wrapped in an Optional, thus null values will not be directly stored in oldValues.

@kris7t
Copy link
Contributor

kris7t commented Jun 19, 2024

Thanks for the contribution!

To be honest, I wouldn't want to add one more layer of indirection (Optional<T>) to aggregators. In the common case, the aggregate result is never null, since aggregators act as outer joins. I updated to the code to clarify this with @NotNull annotations to hopefully make the API semantics more clear for users: ac4f28d

I'd be willing to add another, "nullable" aggregator that can entirely remove matches. This should ideally map to a different RETE node (with extra indirection if needed). However, this isn't a common use-case for Refinery, as entirely removing an aggregator match makes abstract interpreters quite hard to define.

@kris7t kris7t force-pushed the main branch 18 times, most recently from ca29cf5 to d46a69a Compare June 24, 2024 17:27
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.

None yet

2 participants