Avoid repeatedly wrapping DedupConsumer instances - #1291
Merged
Merged
Conversation
The QueryIndex forEach methods call foreEach on other instances, so would cause repeated wrapping of the supplied consumer even though it was already a DedupConsumer. Add a static factory method that returns the passed in instance if it's already a DedupConsumer, and use it in the forEach methods. Update forEachMatchImpl to call forEachMatchImpl instead of forEach to bypass this instanceof check.
kilink
force-pushed
the
dedupe-consumer-fix
branch
from
September 5, 2026 00:24
e495f63 to
3e0be08
Compare
brharrington
approved these changes
Sep 8, 2026
brharrington
added a commit
that referenced
this pull request
Sep 8, 2026
Three changes to the Id based traversal, none of which alter what it returns. They remove waste and two divergences from the equivalent Function based method next to it. The private worker is renamed to forEachMatchImpl, matching the Function one. It was an arity overload of the public forEachMatch, which is the shape that caused the repeated wrapping fixed in #1291: dropping the position argument from a recursive call bound to the public method and re-wrapped the consumer, silently. With the rename that call no longer compiles, so DedupConsumer.from is a guard rather than the mechanism. The has key descent started scanning from the position of the tag that matched this node's key. That sub-tree is built from the queries after the ones for this key, so its own key sorts strictly after keyRef and the tag at that position can never match it: the scan spent one iteration and one compareTagKey on it before moving on. Start past it, as the exact and other-condition descents from the same position already do. The other-keys and missing-keys descents sat inside the "key != null" guard, where the Function method has them outside it. The two agree today because add() assigns key before it can create either sub-tree, so a node with no key has neither, but nothing recorded that and the two methods read as if one of them was wrong. Taking the shape that does not depend on the invariant leaves them symmetric. Also renames the type parameter on DedupConsumer.from from T to V. It shadowed the enclosing DedupConsumer<T>, so the cast read as though it referenced the class parameter; the other static methods on this class use V for the same reason. Tests for the has key descent with several keys following it, with one of those keys absent, and with the matching tag last so the sub-tree has nothing left to scan. They pass before the change as well, since the behaviour is unchanged, but they do constrain the position: an off by one in that descent fails six tests in the class.
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
The QueryIndex forEach methods call foreEach on other instances, so would cause repeated wrapping of the supplied consumer even though it was already a DedupConsumer.
Add a static factory method that returns the passed in instance if it's already a DedupConsumer, and use it in the forEach methods. Update forEachMatchImpl to call forEachMatchImpl instead of forEach to bypass this instanceof check.