-
Notifications
You must be signed in to change notification settings - Fork 159
feat: allow caching the results of an unfiltered UniStream (WIP) #1853
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
base: main
Are you sure you want to change the base?
feat: allow caching the results of an unfiltered UniStream (WIP) #1853
Conversation
- Introduces ConstraintFactory.staticData(Uni), which creates an independent NodeNetwork from the provided Uni stream. - Introduces TupleSourceRoot that represent top level tuple producers (the ForEach nodes now implement this, as well as a new StaticDataUniNode). - The StaticDataUniNode caches the results of its internal NodeNetwork and maps the tuples to new tuples compatiable with the external NodeNetwork - Inserts/retracts invalidate the cache and cause the output tuples to be updated. - Updates uses the cache and do not notify the internal NodeNetwork.
|
// public void beforeProblemPropertyChanged(Object problemFactOrEntity) // Do nothing | ||
@Override | ||
public void beforeProblemPropertyChanged(Object problemFactOrEntity) { | ||
// Since this is called when a fact (not a variable changes), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// Since this is called when a fact (not a variable changes), | |
// Since this is called when a fact (not a variable) changes, |
This is surprisingly simple! Some high-level comments; leaving naming out of the picture for now:
I'm not a big fan of this. The programming model doesn't look very nice IMO.
Something like this would be more stream-y:
This brings its own problems. But both proposals share the same issue - chaining static streams. Nothing (other than run-time fail-fasts) prevents you from doing This was the main idea of |
|
||
@Override | ||
public Propagator getPropagator() { | ||
return new StaticPropagationQueue<>(new TupleLifecycle<>() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this propagates to nowhere? The whole node seems to exist only to record the tuples; arguably, the code should be refactored to not require this to be a node, and just use the recorder directly.
|
||
@NullMarked | ||
public interface TupleSourceRoot<A> { | ||
void insert(A a); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure why this used to be @Nullable
, but I remember clearly that I was adding it there for a reason.
I recommend adding null checks, running the entire CI incl. quickstarts, and if the null checks don't trigger, then I say it's safe for this to be non-null.
invalidateCache(); | ||
tupleMap.put(a, new ArrayList<>()); | ||
insertIntoNodeNetwork(a); | ||
recalculateTuples(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doing this on every insert/retract seems excessively expensive. There will be a large burst of inserts at the beginning; they ought to be cached and processed in a batch, probably before propagation starts.
@Override | ||
public void insert(UniTuple<A> tuple) { | ||
// Do nothing; this is a source node | ||
} | ||
|
||
@Override | ||
public void update(UniTuple<A> tuple) { | ||
// Do nothing; this is a source node | ||
} | ||
|
||
@Override | ||
public void retract(UniTuple<A> tuple) { | ||
// Do nothing; this is a source node | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Arguably if these methods aren't needed, then this shouldn't implement TupleLifecycle
. If it doesn't look like a duck and doesn't quack like a duck, it shouldn't be called a duck.
import ai.timefold.solver.core.impl.score.stream.common.RetrievalSemantics; | ||
import ai.timefold.solver.core.impl.score.stream.common.inliner.AbstractScoreInliner; | ||
|
||
public class BavetStaticDataUniConstraintStream<Solution_, A> extends BavetAbstractUniConstraintStream<Solution_, A> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know that we said we don't want to deal with joins yet. But considering that the same mechanism would be used to support joins as well, I think we deserve to see how this would look for a bi-stream.
(Also, considering that this suddenly looks like much less work than originally estimated, maybe we actually do take joins in scope?)
* As this is cached, it is vital the stream does not reference any variables | ||
* (genuine or otherwise). | ||
*/ | ||
<A> @NonNull UniConstraintStream<A> staticData(UniConstraintStream<A> stream); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens when this stream is used both statically and non-statically?
(Think node-sharing.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The output stream can be node shared if the input streams are identical (not implemented currently since this is a POC). The input stream will not be node shared; its an independent network.
To bring an alternative API idea:
This would produce a In essence, this brings a very simple, clear API:
It does have a downside - it doesn't allow arbitrary static streams, such as groupBy, map, flatten etc. Right now, nobody is asking for it, and I think it is a decent trade-off to get the benefits; but we can possibly ask other people for their opinion on this. |
Introduces ConstraintFactory.staticData(Uni), which creates an independent NodeNetwork from the provided Uni stream.
Introduces TupleSourceRoot that represent top level tuple producers (the ForEach nodes now implement this, as well as a new StaticDataUniNode).
The StaticDataUniNode caches the results of its internal NodeNetwork and maps the tuples to new tuples compatiable with the external NodeNetwork
Inserts/retracts invalidate the cache and cause the output tuples to be updated.
Updates uses the cache and do not notify the internal NodeNetwork.