CoreyKaylor / Lightning.NET

.NET library for LMDB key-value store
Other
397 stars 82 forks source link

Return key and value for LightningCursor's First, Next, Last, Previous methods #168

Closed miroslavp closed 1 month ago

miroslavp commented 8 months ago

Hi guys, First of all, I want to say that I really appreciate the effort that you put in this library!

I have a question: Is there a good reason why the First(), Next(), Previous(), Last() methods in LightningCursor and their DUP variants return only MDBResultCode and not the same tuple (including the key and the value) like in the GetCurrent()?

I can swear that the logic for returning the key and the value is right there in the native library https://git.openldap.org/openldap/openldap/-/blob/mdb.master/libraries/liblmdb/mdb.c

Currently if I want to get the first cursor value, I have to call cursor.First() and then cursor.GetCurrent(). Inside First() we call the native mdb_cursor_get function with MDB_FIRST for MDB_cursor_op which should return the result code as well as the key and the value which makes basically the second call of GetCurrent() unnecessary. Same applies for the other methods like Next(), Previous() and so on.

Can we change the signature of those methods to return the key and the value as well rather than discarding them?

This will avoid duplicating the work (calling Next() and then GetCurrent()) for each element inside AsEnumerable() for example https://github.com/CoreyKaylor/Lightning.NET/blob/fdd8b310778b5f1a63af4ed144260dfc7ef0b39e/src/LightningDB/LightningExtensions.cs#L64-L72

Let me know what you think

CoreyKaylor commented 8 months ago

You're definitely right. This was probably an overlooked aspect when migrating away from the old cursor design that leaned too heavily on IEnumerable and every call would expect GetCurrent.

CoreyKaylor commented 1 month ago

Fixed in e084fa0