Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -490,6 +490,15 @@ else if (documentRange != null && (!this.isContainsIndexOnlyTerms() && this.getT
UniqueTransform uniquify = getUniqueTransform();
if (uniquify != null) {
pipelineDocuments = uniquify.getIterator(pipelineDocuments, this.yieldCallback);
if (range.getStartKey() != null && !WaitWindowObserver.hasMarker(range.getStartKey())) {
// After a teardown the unique transform reloads what it persisted before the rebuild, so it can hand back documents that were already
// returned and sort before the seek range. A tablet server passes such keys straight to the client, so drop them here. Subclasses such
// as AncestorQueryIterator turn the teardown range into an inclusive one before calling this, so the start key's inclusivity is not a
// reliable sign of a rebuild; nothing before the range start belongs in the results either way. 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()));

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added most_recent_unique_after_teardown in QueryIteratorIT, checking for this explicit case

}
}

// apply the grouping iterator if requested and if the batch size is greater than zero
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1344,6 +1344,30 @@ public void most_recent_unique() throws IOException {
event_test(seekRange, query, false, hitOverride, otherData, Collections.EMPTY_LIST, additionalOptions);
}

/**
* A teardown rebuilds the iterator and seeks it again just after the last key it returned. The most recent unique transform reloads the documents it
* persisted before the teardown, and must not hand back the ones that precede the new seek range.
*/
@Test
public void most_recent_unique_after_teardown() throws IOException {
List<Map.Entry<Key,Value>> listSource = configureTestData(11);
listSource.addAll(addEvent("123.345.457", 12));
listSource.addAll(addEvent("123.345.458", 13));
baseIterator = new SortedListKeyValueIterator(listSource);

configureIterator();
options.put(QUERY, "EVENT_FIELD1 == 'a' && EVENT_FIELD4 == 'd' && EVENT_FIELD6 == 'f' && f:most_recent_unique('EVENT_FIELD2')");
options.put(INDEX_ONLY_FIELDS, "");
options.put(UNIQUE_FIELDS, "EVENT_FIELD2[ALL]");
options.put(QueryParameters.MOST_RECENT_UNIQUE, "true");

replayAll();
List<Map.Entry<Key,Document>> results = lookupTask.lookupWithTeardown(baseIterator, options, iterEnv, getShardRange());
verifyAll();

eval(results, Collections.singletonList(getBaseExpectedEvent("123.345.458")));
}

protected void configureIterator() {
lookupTask.setTypeMetadata(typeMetadata);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,44 @@ public List<Map.Entry<Key,Document>> lookup(SortedKeyValueIterator<Key,Value> so
return results;
}

/**
* Run the lookup the way a tablet server does after a teardown: rebuild the iterator from scratch after every result and seek it again starting just after
* the last key it returned. Yields are followed the same way {@link #lookup} follows them.
*
* @return the results, in the order the rebuilt iterators returned them
* @throws IllegalStateException
* if a rebuilt iterator returns a key that does not follow the key it was rebuilt after
*/
public List<Map.Entry<Key,Document>> lookupWithTeardown(SortedKeyValueIterator<Key,Value> source, Map<String,String> options, IteratorEnvironment env,
Range range) throws IOException {
includeGroupingContext = Boolean.parseBoolean(options.getOrDefault(QueryOptions.INCLUDE_GROUPING_CONTEXT, "false"));

List<Map.Entry<Key,Document>> results = new ArrayList<>();
Range r = range;
Key lastResultKey = null;
while (true) {
YieldCallback<Key> yield = new YieldCallback<>();
QueryIterator iterator = init(source, options, env, yield);
iterator.seek(r, Collections.emptyList(), false);
if (iterator.hasTop()) {
Key topKey = iterator.getTopKey();
if (lastResultKey != null && topKey.compareTo(lastResultKey) <= 0) {
throw new IllegalStateException("Iterator rebuilt after " + lastResultKey + " returned " + topKey + ", which does not follow it");
}
Document document = deserializeAndFilterDocument(iterator.getTopValue());
if (!document.getDictionary().isEmpty()) {
results.add(new AbstractMap.SimpleEntry<>(topKey, document));
}
lastResultKey = topKey;
r = new Range(topKey, false, range.getEndKey(), range.isEndKeyInclusive());
} else if (yield.hasYielded()) {
r = new Range(yield.getPositionAndReset(), false, range.getEndKey(), range.isEndKeyInclusive());
} else {
return results;
}
}
}

private Document deserializeAndFilterDocument(Value value) {
Map.Entry<Key,Document> deserializedValue = deserialize(value);
Document d = deserializedValue.getValue();
Expand Down
Loading