-
Notifications
You must be signed in to change notification settings - Fork 94
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
Rework type system #312
Merged
Merged
Rework type system #312
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Co-authored-by: Ivan Trubach <[email protected]>
…genji into merge-key-and-document-packages
Codecov Report
@@ Coverage Diff @@
## master #312 +/- ##
==========================================
+ Coverage 61.37% 61.78% +0.40%
==========================================
Files 73 74 +1
Lines 6701 6837 +136
==========================================
+ Hits 4113 4224 +111
+ Misses 2053 2046 -7
- Partials 535 567 +32
Continue to review full report at Codecov.
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR makes an important change in Genji's type system and indexes.
Background
Currently, Genji parses integers and doubles based on their literal representations:
This means that the same table can have both integers and doubles depending on how values have been parsed.
Initially, comparison operators were very strict: only values of the same type can be compared together.
However, this led to situations like this:
Because
0
is parsed as an integer, it was only compared to integers, skipping double values.We then changed the behavior of comparison operators to support an edge case: If we compare integers and doubles, convert the integer to a double prior comparison.
Since comparison operators are also used for ordering, we expect the following behavior:
However, this kind of behavior is also expected if
a
is indexed:And this is very complex. As of today, we still don't have proper support for the above requirements.
This led for example to the creation of the
binarysort
package (formerkey
package) which encodes integers and doubles on 16 bytes. While this works, this adds too much complexity and performance issues during index iteration, while obviously taking much more space.Solution
The solution proposed here is to change the rules: numbers are stored as doubles by default
The parser doesn't change, integers are scanned as integers, doubles as doubles.
Comparison and arithmetic operators keep the same behavior.
The only difference is that right before storing the document, every integer field will be converted to double if there is no explicit constraint on that field.
This means that one field can never be both a double or an integer on two different documents:
This new property allows us to simplify index implementation and to remove all the edge cases.