GeezerVison is showing github.com — ByteBuffer sorting changed in 0.9.0 · Issue #230 · lmdbjava/lmdbjava
ByteBuffer sorting changed in 0.9.0 #230DescriptionHi
I think 0.9.0 introduced regression where ByteBuffers comparison has changed.
This likely is a result of a change in #199
where custom comparator for byte[] and ByteBuffer has been swapped for Arrays.compare(o1, o2);
It seems original implementation was always using unsigned bytes.
Now default implementation uses unsigned bytes.
As far as I understand this can be modified by passing MDB_INTEGERKEY to _env.openDb and this is a setting persisted in Db.
This was never a requirement in previous versions so it seems existing databases are not compatible unless the flag is added in the code.
When I try to add that flag my tests still fail unable to either get key/value or iterate through them (iterator is empty).
This is likely related to
#228 Reacted by Cosimo Damiano Prete, Cameron Sumpter, Samuel Tschiedel and at055612 Activityivenhov commented on Jan 22, 2024 Just confirmed that dropping commit 3524995 brings back previous behaviour and my test pass.
In case it's relevant when I'm opening database I do
Dbi<ByteBuffer> db = _env.openDbi(hexId, MDB_CREATE);
ivenhov commented on Jan 22, 2024 Further tests show that if I open db like this
Comparator<ByteBuffer> cmp = new MyComparatorCopiedFromByteBufferProxy();
Dbi<ByteBuffer> db = _env.openDbi(hexId, cmp, MDB_CREATE);
where MyComparatorCopiedFromByteBufferProxy has implementation copied from org.lmdbjava.ByteBufferProxy$AbstractByteBufferProxy then my test are passing as well on 0.9.0
private static class MyComparatorCopiedFromByteBufferProxy implements Comparator<ByteBuffer> {
@Override
public int compare(ByteBuffer o1, ByteBuffer o2) {
return compareBuff(o1, o2);
}
/**
* Lexicographically compare two buffers.
*
* @param o1 left operand (required)
* @param o2 right operand (required)
* @return as specified by {@link Comparable} interface
*/
public static int compareBuff(final ByteBuffer o1, final ByteBuffer o2) {
requireNonNull(o1);
requireNonNull(o2);
if (o1.equals(o2)) {
return 0;
}
final int minLength = Math.min(o1.limit(), o2.limit());
final int minWords = minLength / Long.BYTES;
final boolean reverse1 = o1.order() == LITTLE_ENDIAN;
final boolean reverse2 = o2.order() == LITTLE_ENDIAN;
for (int i = 0; i < minWords * Long.BYTES; i += Long.BYTES) {
final long lw = reverse1 ? reverseBytes(o1.getLong(i)) : o1.getLong(i);
final long rw = reverse2 ? reverseBytes(o2.getLong(i)) : o2.getLong(i);
final int diff = Long.compareUnsigned(lw, rw);
if (diff != 0) {
return diff;
}
}
for (int i = minWords * Long.BYTES; i < minLength; i++) {
final int lw = Byte.toUnsignedInt(o1.get(i));
final int rw = Byte.toUnsignedInt(o2.get(i));
final int result = Integer.compareUnsigned(lw, rw);
if (result != 0) {
return result;
}
}
return o1.remaining() - o2.remaining();
}
}
ivenhov commented on Jan 22, 2024 It looks like the problem is related to changing what is a default comparator for ByteBuffer.
In pre 0.9.0 it was a custom lmdbjava implementation which was doing unsigned comparison.
In 0.9.0 comparator is selected based on the flag MDB_INTEGERKEY
See method in ByteBufferProxy
@Override
protected Comparator<ByteBuffer> getComparator(final DbiFlags... flags) {
final int flagInt = mask(flags);
if (isSet(flagInt, MDB_INTEGERKEY)) {
return this::compareCustom;
}
return this::compareDefault;
}
In my case, since I'm not passing MDB_INTEGERKEY new comparator is used based on ByteBuffer compareTo (signed bytes)
protected final int compareDefault(final ByteBuffer o1,
final ByteBuffer o2) {
return o1.compareTo(o2);
}
This in itself is a behaviour change which breaks lmdbjava for me.
Supplying MDB_INTEGERKEY also does not work, not clear to me why
But I was under the impression MDB_INTEGERKEY should be used where keys are actually integers, not byte[]
From DbiFlags
/**
* Numeric keys in native byte order: either unsigned int or size_t. The keys
* must all be of the same size.
*/
MDB_INTEGERKEY(0x08),
Not sure if supplying that flag has some side effects in native lmdb library.
As mentioned before a workaround for me is to use a comparator which is a copy of the code from AbstractByteBufferProxy ivenhov commented on Feb 8, 2024 Hi
No, it was not working, see my comment
…Reacted by Cosimo Damiano Prete cdprete commented on Mar 6, 2024 ivenhov commented on Mar 8, 2024 Hi @cdprete
I took your pull request and added extra flag so the open looks like
Dbi<ByteBuffer> db = _env.openDbi(hexId, MDB_CREATE, MDB_UNSIGNEDKEY);
I guess that's how it's meant to be used?
All my tests passed with flying colours
Looks like a solid fix. Well done Reacted by Samuel Tschiedel cdprete commented on Mar 9, 2024
Hi @cdprete I took your pull request and added extra flag so the open looks like
Dbi<ByteBuffer> db = _env.openDbi(hexId, MDB_CREATE, MDB_UNSIGNEDKEY);
I guess that's how it's meant to be used? All my tests passed with flying colours Looks like a solid fix. Well done
Hi @ivenhov
Yes, that's how it should be used.
Thank you for testing it out. cdprete commented on Mar 9, 2024 Hi @ivenhov.
May I ask you to test it again?
I've realized in some spots I missed to skip the additional flag (fixed now). :)
Also, have a read at #237 (comment) ;) ivenhov commented on Mar 9, 2024 Updated pull request #237 still passes my tests when flag MDB_UNSIGNEDKEY is added, so all good.
I'm just thinking that this should be default since that was the original behaviour as far as I know and I don't see why anyone would care about/compare bytes signed. Possibly slightly more performant? cdprete commented on Mar 9, 2024 Hi.
I agree (so, to add a MDB_SIGNEDKEY flag instead to still support both), but it would then yet be another breaking change when compared against v0.9.0.
Of course, it's up to @benalexau to decide.
Possibly slightly more performant?
I doubt.
On the other end, after checking how the Dbi is opened, I'm a bit puzzled on why the comparator is used to begin with, since the flag nativeCb is false and it should therefore be ignored. thomasjtaylor commented on Mar 29, 2024 I came across this same issue upgrading from 0.8.3 where our LMDB tables used ByteBuffer.putLong( _rowId ) for the keys. When we tried to retrieve _rowId=127 (Byte.MAX_VALUE), the new Comparator triggers CursorIterable TERMINATE instead of RELEASE, causing no value being returned, and the iterator ending... it also occurred at something like _rowId = 2^8+127, so the default 50-row cache would be periodically empty.
When I compared branches [0.9.0, 0.8.3] and found the #199 update, I added DbiFlags.MDB_INTEGERKEY to my .openDbi() call and it resolved my issue.
Maybe the ChangeLog or README.md could be updated to document this new feature? cdprete commented on Apr 17, 2024
I came across this same issue upgrading from 0.8.3 where our LMDB tables used ByteBuffer.putLong( _rowId ) for the keys. When we tried to retrieve _rowId=127 (Byte.MAX_VALUE), the new Comparator triggers CursorIterable TERMINATE instead of RELEASE, causing no value being returned, and the iterator ending... it also occurred at something like _rowId = 2^8+127, so the default 50-row cache would be periodically empty.
When I compared branches [0.9.0, 0.8.3] and found the #199 update, I added DbiFlags.MDB_INTEGERKEY to my .openDbi() call and it resolved my issue.
Maybe the ChangeLog or README.md could be updated to document this new feature?
Hello.
MDB_INTEGERKEY works indeed only with numeric keys, so adding it will definitively solve your issue.
On the other end, for me and @ivenhov that we use a bunch of bytes as key (byte array, byte buffer, whatsoever) that flag doesn't help because the key isn't just a number.
bp-alex commented on Feb 17, 2025 Thanks for your separate PR, which I merged earlier today. Would you please give it a try with the 0.9.1-SNAPSHOT and close this ticket if fixed? bp-alex commented on Feb 20, 2025 I have released 0.9.1 so I will close this ticket in anticipation PR #237 corrected it. If the problem is outstanding in 0.9.1 please open a new ticket so we can track it. Thanks.
MetadataMetadataDevelopmentNo branches or pull requestsIssue actions
|