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

CursorIterable/KeyRange broken in v0.9.0 #228

Open
at055612 opened this issue Dec 11, 2023 · 2 comments · May be fixed by #237
Open

CursorIterable/KeyRange broken in v0.9.0 #228

at055612 opened this issue Dec 11, 2023 · 2 comments · May be fixed by #237

Comments

@at055612
Copy link
Contributor

The changes in v0.9.0 have broken CursorIterable for some KeyRange types and keys. I noticed the problem when using KeyRange.atLeastBackward(. The cursor was starting on the wrong key.

I think the problem is this line:

lmdbjava-0.8.3...lmdbjava-0.9.0#diff-a5dda1cdb60b45d45a5a4364768ad1f7c62ac0ec025f295f78d489cf08392bf2R206

It is using the standard java ByteBuffer.compareTo method rather than your lexographical AbstractByteBufferProxy::compareBuff method, so 150 is considered < 110.

See my branch (on the link below) where I have changed your CursorIterableTest to use 200 ... 900 instead of 2 ... 9 for the key values to demonstrate the problem. With the modified values a lot of the tests fail. Changing AbstractByteBufferProxy to use compareBuf makes them all pass again.

https://github.com/lmdbjava/lmdbjava/compare/master...at055612:backwards-at-least?expand=1

I think this is the problem is in AbstractByteBufferProxy.

    @Override
    protected Comparator<ByteBuffer> getComparator(final DbiFlags... flags) {
      final int flagInt = mask(flags);
      if (isSet(flagInt, MDB_INTEGERKEY)) {
        return this::compareCustom;
      }
      return this::compareDefault;
      // TODO Think the above line should be changed to this:
      // return AbstractByteBufferProxy::compareBuff;
    }

It may also be an issue for netty/agrona but I am not using those so have not looked.

This is a pretty serious bug for anyone using KeyRanges and it will present in subtle ways depending on the keys in the db and the keyRange type used.

@cdprete
Copy link

cdprete commented Jan 18, 2024

I can confirm as well that the KeyRanges are somehow broken. In my case I'm using KeyRange#closed.

@cdprete
Copy link

cdprete commented Mar 6, 2024

The changes in v0.9.0 have broken CursorIterable for some KeyRange types and keys. I noticed the problem when using KeyRange.atLeastBackward(. The cursor was starting on the wrong key.

I think the problem is this line:

lmdbjava-0.8.3...lmdbjava-0.9.0#diff-a5dda1cdb60b45d45a5a4364768ad1f7c62ac0ec025f295f78d489cf08392bf2R206

It is using the standard java ByteBuffer.compareTo method rather than your lexographical AbstractByteBufferProxy::compareBuff method, so 150 is considered < 110.

See my branch (on the link below) where I have changed your CursorIterableTest to use 200 ... 900 instead of 2 ... 9 for the key values to demonstrate the problem. With the modified values a lot of the tests fail. Changing AbstractByteBufferProxy to use compareBuf makes them all pass again.

https://github.com/lmdbjava/lmdbjava/compare/master...at055612:backwards-at-least?expand=1

I think this is the problem is in AbstractByteBufferProxy.

    @Override
    protected Comparator<ByteBuffer> getComparator(final DbiFlags... flags) {
      final int flagInt = mask(flags);
      if (isSet(flagInt, MDB_INTEGERKEY)) {
        return this::compareCustom;
      }
      return this::compareDefault;
      // TODO Think the above line should be changed to this:
      // return AbstractByteBufferProxy::compareBuff;
    }

It may also be an issue for netty/agrona but I am not using those so have not looked.

This is a pretty serious bug for anyone using KeyRanges and it will present in subtle ways depending on the keys in the db and the keyRange type used.

Could you maybe try with #237?
For some yet unknow reasons, I'm not able to build and run it on my Windows machine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants