-
Notifications
You must be signed in to change notification settings - Fork 232
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
Start validation #423
Start validation #423
Conversation
It looks like the tests fail. Can you have a look? This is quite good overall. See my minor comments. Very useful contribution, |
It could be interesting to try to deserialize garbage content and check that you do get errors. |
Could you help me understand this invariant
What is distinct values here? |
@lemire Anything else on this? I'm going on holiday and am closing out my open pr's |
@bearrito If you recommend merging this, I will!!! |
@lemire Can you comment on my question above on the run-length invariant. I think I can get one more set of tests of that if I understand better. After that, I'll feel good merging. |
|
Counting the number of distinct values in a run container is basically just the sum of the lengths of the runs.... (remember that the runs are non-overlapping) |
Added in distinct value sum/run cardinality logic. I don't think I can add anymore test pressure on this. |
Running tests. I expect to merge. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please consider @Oppen 's additional checks.
@lemire will do. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome!
Running tests. |
Merging. |
@lemire