Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Shorter encoding for numbers (breaking change) #9

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

dominictarr
Copy link
Owner

@PaulBlanche I realized that numbers are encoded with a huge amount of precision.
2 is encoded as FE500M2.00000000000000000000 ... that seems unnecessary to me?

I removed that, and noticed it failed on the close random number tests

so I tried just appending a number that we know is higher than 9, but only on negative numbers. This seems to work, tests pass.
cw.encode(-1)+'a' > cw.encode(-1.00000001)+'a'

@dominictarr
Copy link
Owner Author

I chose '_' as the end character, since it's higher than numbers, and makes the output more readable.

@PaulBlanche
Copy link
Contributor

Yes indeed, numbers where encoded as "fixed length record". This was needed for the digit flipping trick to work on negative numbers.

Adding a char superior to 9 to the mantissa of a negative number should work, that's a nice idea !

codec/number.js Outdated
@@ -14,8 +16,8 @@ exports.encode = function (number) {

var splitScientificNotation = number.toExponential().split('e');
var exponent = Number(splitScientificNotation[1]) + 500;
var mantissa = splitScientificNotation[0] + (splitScientificNotation[0].indexOf('.') === -1 ? '.' : '') + '0'.repeat(20);
var encoded = 'E' + padStart(String(exponent), 3) + 'M' + String(mantissa);
var mantissa = splitScientificNotation[0] + (splitScientificNotation[0].indexOf('.') === -1 ? '.' : '')//+ '0'.repeat(20);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the zeros are not appended anymore, there is no need to add a '.' after the mantissa if it is a round number

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

@Raynos
Copy link
Contributor

Raynos commented Mar 10, 2020

Is there any desire to merge this compact change ?

@Raynos
Copy link
Contributor

Raynos commented Mar 10, 2020

I went ahead and publishes this breaking change by making a new package and naming it charwise-compact ( https://github.com/Raynos/charwise-compact ).

@dominictarr
Copy link
Owner Author

dominictarr commented Mar 10, 2020 via email

@PaulBlanche
Copy link
Contributor

From what i remember, everything was ok.

@Raynos
Copy link
Contributor

Raynos commented Mar 11, 2020

I suspect you either forgot about this or did not want to do any breaking changes or migrations of the key space in leveldb.

@@ -6,6 +6,8 @@
// We endpad mantissa with enough zero to exceed mantissa precision.
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this comment is for the old code...

@Raynos
Copy link
Contributor

Raynos commented Mar 13, 2020

You have reviewed your own pull request. This is pretty meta.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants