timshannon / badgerhold

BadgerHold is an embeddable NoSQL store for querying Go types built on Badger
MIT License
520 stars 51 forks source link

Use uint64 in countQuery, TxCount, Count() #57

Closed NateWilliams2 closed 3 years ago

NateWilliams2 commented 3 years ago

Right now, countQuery (and thereby the exposed Count() and TxCount()) uses an int to count records, meaning only ~2 billion records are supported before the count could overflow.

This pr uses uint instead, increasing that value to ~10^19.

timshannon commented 3 years ago

Only on 32bit machines though right? It'll be 64bit if the machine is 64 bit.

NateWilliams2 commented 3 years ago

Yeah, but this ensures it's 64 bit on either machine. And afaik there's no reason for it to be a signed int

timshannon commented 3 years ago

Except for the fact that it would break backwards compatibility. And for breaking that backwards compatibility we gain the ability to return more than 2 billion records on 32bit machines? Is that even a use case? Doesn't seem worth it IMO.

On Fri, Apr 9, 2021 at 3:46 PM Nate Williams @.***> wrote:

Yeah, but this ensures it's 64 bit on either machine. And afaik there's no reason for it to be a signed int

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/timshannon/badgerhold/pull/57#issuecomment-816962537, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAKE2NIDDCZJXAV3IXJJKK3TH5RT7ANCNFSM42VT65TQ .

-- Tim Shannon www.townsourced.com

NateWilliams2 commented 3 years ago

Understandable. Do you anticipate any other breaking changes you could release this one with? Obviously isn’t urgent but would be nice to have.

timshannon commented 3 years ago

Yeah, Badger has bumped the version and broken compatibility in the past, I could move this up with the next one.

Thanks for the PR.

On Fri, Apr 9, 2021 at 4:39 PM Nate Williams @.***> wrote:

Understandable. Do you anticipate any other breaking changes you could release this one with? Obviously isn’t urgent but would be nice to have.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/timshannon/badgerhold/pull/57#issuecomment-816986300, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAKE2NNT7RVACDCAB6FKBALTH5X2TANCNFSM42VT65TQ .

-- Tim Shannon www.townsourced.com

NateWilliams2 commented 3 years ago

Thanks :)

timshannon commented 3 years ago

I'll be merging this in with #70 as that is going to also be a backwards incompatible change, and we'll rev the module version to 4.

Thanks,