Skip to content
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

MergingIterator#resetPriorityQueue is bogus. #95

Open
pcmind opened this issue Jan 12, 2018 · 0 comments
Open

MergingIterator#resetPriorityQueue is bogus. #95

pcmind opened this issue Jan 12, 2018 · 0 comments

Comments

@pcmind
Copy link
Contributor

pcmind commented Jan 12, 2018

If MergingIterator#seekToFirst() or MergingIterator#seek(key) are called after starting using the iterator (after calling first #next()), MergingIterator start returning wrong values.

Root cause:
resetPriorityQueue() should clear queue before reusing it, other wise duplicate iterators will be inserted and used in next iterations

Following unit test reproduce the problem:


@Test
public void testName() throws Exception {
    InternalKeyComparator comparator = new InternalKeyComparator(new BytewiseComparator());
    InternalKey key1 = new InternalKey(Slices.wrappedBuffer(new byte[]{'1'}), 1, ValueType.VALUE);
    InternalKey key2 = new InternalKey(Slices.wrappedBuffer(new byte[]{'3'}), 2, ValueType.VALUE);
    InternalKey key3 = new InternalKey(Slices.wrappedBuffer(new byte[]{'2'}), 3, ValueType.VALUE);
    InternalKey key4 = new InternalKey(Slices.wrappedBuffer(new byte[]{'4'}), 4, ValueType.VALUE);
    ImmutableList<Map.Entry<InternalKey, Slice>> of1 = ImmutableList
            .of(
                    Maps.immutableEntry(key1, Slices.EMPTY_SLICE),
                    Maps.immutableEntry(key2, Slices.EMPTY_SLICE)
            );
    ImmutableList<Map.Entry<InternalKey, Slice>> of2 = ImmutableList
            .of(
                    Maps.immutableEntry(key3, Slices.EMPTY_SLICE),
                    Maps.immutableEntry(key4, Slices.EMPTY_SLICE)
            );
    ImmutableList<InternalIterator> of = ImmutableList.of(new InternalKeySliceAbstractSeekingIterator(of1, comparator), new InternalKeySliceAbstractSeekingIterator(of2, comparator));
    MergingIterator mergingIterator = new MergingIterator(of, comparator);
    assertEquals(mergingIterator.next().getKey(), key1);
    assertEquals(mergingIterator.next().getKey(), key3);
    mergingIterator.seekToFirst();
    assertEquals(mergingIterator.next().getKey(), key1);
    assertEquals(mergingIterator.next().getKey(), key3);
    assertEquals(mergingIterator.next().getKey(), key2);
    assertEquals(mergingIterator.next().getKey(), key4); //fail here! key2 is return a second time!
    assertFalse(mergingIterator.hasNext());


}

private static class InternalKeySliceAbstractSeekingIterator extends AbstractSeekingIterator<InternalKey, Slice> implements InternalIterator {
    int index = 0;
    private final List<Map.Entry<InternalKey, Slice>> entries;
    private final InternalKeyComparator comparator;

    public InternalKeySliceAbstractSeekingIterator(List<Map.Entry<InternalKey, Slice>> entries, InternalKeyComparator comparator) {
        this.entries = entries;
        this.comparator = comparator;
    }

    @Override
    protected void seekToFirstInternal() {
        index = 0;
    }

    @Override
    protected void seekInternal(InternalKey targetKey) {
        index = entries.size();
        for (int i = 0; i < entries.size(); i++) {
            Map.Entry<InternalKey, Slice> entry = entries.get(i);
            if (comparator.compare(entry.getKey(), targetKey) >= 0) {
                index = i;
            }
        }
    }

    @Override
    protected Map.Entry<InternalKey, Slice> getNextElement() {
        return index < entries.size() ? entries.get(index++) : null;
    }
}
pcmind added a commit to pcmind/leveldb that referenced this issue Jan 13, 2018
pcmind added a commit to pcmind/leveldb that referenced this issue Jan 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant