Binary representation of UInt indexes should sort properly.

[jakedt]

We're using memdb with an id field that's supposed to be monotonically increasing. When the value of the field crosses certain binary-relevant thresholds the ordering gets out of whack. For example on an iteration, 256 shows up before 255.

This PR makes the bytes representation of all types of uints sortable.

[hashicorp-cla]

All committers have signed the CLA.

[jzelinskie]

cc @banks

Do you think this change would break anything in consul?

[banks]

Hey, thanks for the PR!

It would be really useful if we could have the problem statement filled out in the PR description here (or link to an issue with the details) - IIRC Varint encoding does sort naturally on its own, can you give an example where they don't? Is it only when used as part of a composite index?

On the same note, adding a test case that fails with the existing implementation would be helpful.

I don't think this would break anything for Consul or other consumers given that the expectation is for integer fields to be sorted already, but would be great to understand how that is being violated! I don't imagine any callers should be reliant on the actual memory encoding of the index. I guess the one possible side effect I can see here is that in the worst case where only small numbers were indexed but the actual underlying type is int or int64 this could consume up to 7 bytes more RAM per key for the same workload. My guess is that's not a big enough problem if it's fixing broken behaviour though.

[jakedt]

Hey, thanks for the PR!

It would be really useful if we could have the problem statement filled out in the PR description here (or link to an issue with the details) - IIRC Varint encoding does sort naturally on its own, can you give an example where they don't? Is it only when used as part of a composite index?

Varint may sort naturally (although that's suspect based on #77) but UVarInt definitely doesn't:
https://play.golang.org/p/an3xS-qlaEZ

On the same note, adding a test case that fails with the existing implementation would be helpful.

Added

I don't think this would break anything for Consul or other consumers given that the expectation is for integer fields to be sorted already, but would be great to understand how that is being violated! I don't imagine any callers should be reliant on the actual memory encoding of the index. I guess the one possible side effect I can see here is that in the worst case where only small numbers were indexed but the actual underlying type is int or int64 this could consume up to 7 bytes more RAM per key for the same workload. My guess is that's not a big enough problem if it's fixing broken behaviour though.

Because the index is allocating the same size buffer for each value based on the type of the value being indexed, this should strictly use less memory in all cases. uint8 takes two bytes when uvarint encoded, this takes 1. uint64 takes 10 bytes when varint encoded, this will use 8.

[banks]

@jakedt Thanks for that and the super quick response!

That extra test looks good and I missed that we allocate a max size buffer even with Varint.

Was going to approve and merge but I spotted one super tiny nit - if you agree we can get that suggestion in and then I'll merge.

Thanks again!

[banks]

Thanks @jakedt!

[banks]

Tagged this in v1.3.2