-
Notifications
You must be signed in to change notification settings - Fork 132
Add support for 64-bit integers for FastPFor #55
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
Conversation
xndai
commented
Jun 18, 2019
- Add a new template parameter for FastPFor encoder to specify the integer type. Curently uint32_t and uint64_t are supported.
- Add corresponding bit packing and unpacking functions for 64-bit integers.
- Overload IntegerCODEC interfaces to provide 64bit version of encodeArray() and decodeArray(). For codes that don't support 64bit, those would just throw not implemented exception.
- Add 64-bit support for CompositeCodec and VariableByte, so the encoding and decoding can work end to end with arbitary number of integers.
- Add gtest to the CMake project and create unittests for the new 64 bit support.
- Replace some of the assertions into exceptions.
1. Add a new template parameter for FastPFor encoder to specify the integer type. Curently uint32_t and uint64_t are supported. 2. Add corresponding bit packing and unpacking functions for 64-bit integers. 3. Overload IntegerCODEC interfaces to provide 64bit version of encodeArray() and decodeArray(). For codes that don't support 64bit, those would just throw not implemented exception. 4. Add 64-bit support for CompositeCodec and VariableByte, so the encoding and decoding can work end to end with arbitary number of integers. 5. Add gtest to the CMake project and create unittests for the new 64 bit support. 6. Replace some of the assertions into exceptions.
|
Looks good to me. Merging. |
| target_link_libraries(unit FastPFor) | ||
| add_custom_target(check unit DEPENDS unit) | ||
|
|
||
| add_executable(fastpor_unittest |
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.
fastpor_unittest? Maybe FastPFor_unittest?..
|
|
||
| add_executable(fastpor_unittest | ||
| unittest/test_composite.cpp | ||
| unittest/test_driver.cpp |
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.
you don't need test_driver.cpp if you are linking to gtest_main
|
Overall, the change completely breaks windows build. Probably better to hard-revert before others pull the change. It doesn't even pass the tests that were added. You cannot even complete them in reasonable amount of time because it prints extreme amount of errors: and at the end result looks like this: @xndai Can you please review your change and possibly fix the commit? I added a few comments, just wanted to do a quick review and it looked like this change cannot possibly work on windows, and sure enough after trying to run the tests I get bad results. Also, tests shouldn't spew endless errors, and perhaps they even don't print proper values (all of them seem to be 32-bit long in output). |
|
Ok. I will revert. |
|
@xndai you'll need these overloads at least: and all other 1UL vs 1ULL fixes (use template to deduce proper one, or make a const at class level?) |
|
I have opened an issue regarding CI tests under Windows... we need help setting it up... |
