[Discussion]: Support Refresh changes for Aggregation operators #1031
JakenVeina
started this conversation in
General
Replies: 1 comment 1 reply
-
My original use case for creating the aggregation operators was that it has to be super fast because I had dozens to consume in an already very busy system. That's why I made them stateless. I've not revisited the code since, and I agree, for correctness it should support refresh changes. I have no issue, with changing this and potentially getting rid of IAggregateChangeSet altogether. I see this as a minor part of what is core to the lib, and have no issue with any breaking changes. |
Beta Was this translation helpful? Give feedback.
1 reply
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
-
Fundamentally, I don't see any real reason we can't or shouldn't support this, but we don't currently.
At a glance, though, I think the reason we don't is a flawed API that all the operators are built on, I.E.
IAggregateChangeSet<>
It only supports 2 types of aggregation changes,AggregateType.Add
andAggregateType.Remove
. Furthermore, because only these types of changes are modeled, the core operator that all the others are built on,.ForAggregation()
assumes that aggregation can always be stateless (I.E. no need to keep an internal cache of items), which is fundamentally untrue forRefresh
changes.I could be missing some functionality in the
Aggregation
namespace, but it seem like we potentially need to make breaking or large-scale API changes here to get this support.I haven't dug too deep into all the code in that namespace yet, but I figured it'd still be worth opening a discussion.
@RolandPheasant @dwcullop Thoughts?
Beta Was this translation helpful? Give feedback.
All reactions