Repository navigation
Do not return documents before the seek range after a unique rebuild - #3974
Open
SethSmucker wants to merge 3 commits into
Open
SethSmucker wants to merge 3 commits into
SethSmucker wants to merge 3 commits into
Conversation
apmoriarty
reviewed
Oct 7, 2026
| // returned and sort before the seek range. A tablet server passes such keys straight to the client, so drop them here. A yield resume | ||
| // is left alone: its start key is the yield marker, and documents before it have not been returned yet. | ||
| final Range seekRange = range; | ||
| pipelineDocuments = Iterators.filter(pipelineDocuments, entry -> !seekRange.beforeStartKey(entry.getKey())); |
Collaborator
There was a problem hiding this comment.
It's a known bug that the most recent unique function can return keys out of order. For a change as significant as this paired with the commit message comment about a hanging test, I would expect to see at least one example of a test that fails without this change going from red to green.
Collaborator
Author
There was a problem hiding this comment.
Added most_recent_unique_after_teardown in QueryIteratorIT, checking for this explicit case
Collaborator
|
Should this bugfix get pointed against the integration branch instead since it also applies there? |
SethSmucker
added
the
Accumulo 4
When a scan is torn down and rebuilt, the most-recent unique transform reloads the documents it persisted before the rebuild and hands them back from the start, including ones already returned. InMemoryScanner used to drop those keys with a range filter, which #3803 removed because a tablet server does not. This filters them in QueryIterator instead. Yield resumes are left alone, since their start key is the yield marker and the documents before it are still owed.
Add TestLookupTask.lookupWithTeardown, which rebuilds the iterator after every result and seeks it again just after the last key, as a tablet server does after a teardown, and fails if a rebuilt iterator returns a key that does not follow it. QueryIteratorIT.most_recent_unique_after_teardown runs the most_recent_unique query through it: without the QueryIterator change the rebuilt iterator returns the same document again, with it the test passes. It drives QueryIterator directly, so it does not depend on InMemoryScanner.
SethSmucker
changed the base branch from
accumulo4-2026-quickstart
to
integration
October 8, 2026 16:44
SethSmucker
force-pushed
the
a4qs/unique-rebuild-range
branch
from
October 8, 2026 16:44
664ec4a to
17f394e
Compare
Collaborator
Yes, it should. Thank you @SethSmucker for targeting integration |
lbschanno
removed
the
Accumulo 4
AncestorQueryIterator turns a teardown's exclusive range into an inclusive one starting just past the returned document before calling QueryIterator.seek, so the filter never applied and AncestorQueryIteratorIT, which inherits most_recent_unique_after_teardown, still saw the document returned again. Apply the filter whenever the seek start is not a yield marker; nothing before the range start belongs in the results either way.
This branch has not been deployed
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
3 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.
Fixes #3973
In
QueryIterator.seek, after applying the unique transform, drop any entry that sorts before the seek range start. After a teardown the unique transform reloads documents it already returned, and nothing before the range start belongs in the results anyway. This covers subclasses such asAncestorQueryIterator, which turn the teardown range into an inclusive one before callingQueryIterator.seek. Yield resumes are left alone: their start key is the yield marker, and the documents before it have not been returned yet.QueryIteratorIT.most_recent_unique_after_teardowncovers it:TestLookupTask.lookupWithTeardownrebuilds the iterator after every result and seeks again just past the last key, as a tablet server does after a teardown, and fails if a rebuilt iterator returns a key that does not follow it. Without this change the rebuilt iterator returns the same document again; with it the test passes. It drivesQueryIteratordirectly, so it does not depend onInMemoryScanner.Tested on
integration(JDK 11) and onaccumulo4-2026-quickstart: the fullquery-coreunit suite passes on integration (7,373 tests), includingQueryIteratorITand its four subclasses (Ancestor, TLD and both WaitWindow ITs) andUniqueTest. On the a4 branch, which builds in-memory-accumulo from source, this also stopsUniqueTest.testMostRecentUniquenessWithIteratorfrom hanging.