Skip to content
This repository was archived by the owner on Dec 1, 2024. It is now read-only.

Support more than 32bit ApproximateSize bindings #776

Closed
larskuhtz opened this issue Jul 26, 2021 · 4 comments · Fixed by #777
Closed

Support more than 32bit ApproximateSize bindings #776

larskuhtz opened this issue Jul 26, 2021 · 4 comments · Fixed by #777
Labels
bug Something isn't working semver-patch Bug fixes that are backward compatible

Comments

@larskuhtz
Copy link
Contributor

Currently the binding for approximateSize stores the result as uint32 (

napi_create_uint32(env_, (uint32_t)size_, &argv[1]);
). This supports only databases of up to 4GB and returns wrong results for larger DBs.

It would be nice if approximateSize could either

  1. support all of Number.isSafeInteger(),
  2. support larger numbers by representing them as nearest float (it's "approximate" after all),
  3. return BitInt, or
  4. at least return an error if an invalid result is returned.

Not sure, which of the options would be best.

@vweevers
Copy link
Member

I prefer option 1. Brings us into petabyte or exabyte territory, without a breaking change.

@vweevers vweevers added enhancement New feature or request bug Something isn't working semver-patch Bug fixes that are backward compatible and removed enhancement New feature or request labels Jul 26, 2021
@larskuhtz
Copy link
Contributor Author

With option (1.), how should the case be handled when then database returns a value that is to big? Throw an error, return -1, or just return whatever napi_create_int64 gives (essentially using option 2. as fallback)?

Currently, it just overflows, which isn't ideal.

@vweevers
Copy link
Member

With option (1.), how should the case be handled when then database returns a value that is to big? Throw an error, return -1, or just return whatever napi_create_int64 gives (essentially using option 2. as fallback)?

Return whatever napi_create_int64 gives. Because as you pointed out, the db size already is approximate. And if you have a database that big, I'm not sure LevelDB is the right fit anyway.

@larskuhtz
Copy link
Contributor Author

larskuhtz commented Jul 27, 2021

cf. PR #777

vweevers pushed a commit that referenced this issue Aug 1, 2021
By using napi_create_int64 instead of napi_create_int32.

Closes #776.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working semver-patch Bug fixes that are backward compatible
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants