Frequency partitioning and FOR encoding rebased & synced with nimble/main#636
Frequency partitioning and FOR encoding rebased & synced with nimble/main#636David-C-L wants to merge 30 commits intofacebookincubator:mainfrom
Conversation
…add frequency partition encoding type getters for test utils
|
Hi @David-C-L! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks! |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
|
Thank you, @David-C-L! Will review and update cc: @zzhao0 |
|
@srsuryadev has imported this pull request. If you are a Meta employee, you can view this in D99487883. |
srsuryadev
left a comment
There was a problem hiding this comment.
Thanks for the initial version @David-C-L, added some initial comments
| for (uint32_t i = 0; i < rowCount; ++i) { | ||
| output[i] = decodeValue(currentRow_ + i); | ||
| } | ||
|
|
There was a problem hiding this comment.
We can see if we can explore bulk processing here, but need not to be in this PR though! For now, it is okay
There was a problem hiding this comment.
I think the bulk processing shouldn't be too hard to implement. I'll give it a quick go and if it's not too much I'll add it to the PR
There was a problem hiding this comment.
I've created a decodeRange function to implement bulk processing in 825c731
There was a problem hiding this comment.
Thank you! Can we update the description with the decode time comparison as well
| // operations for efficient random access. | ||
| Prefix = 11, | ||
| // Partitions data by value frequency. Frequent values get shorter bit-width | ||
| // codes. Rows are reordered to group values with same code length. |
There was a problem hiding this comment.
@David-C-L Can we see if we can achieve better performance without re-ordering the rows?
There was a problem hiding this comment.
The row reordering is to enable efficient value-granularity random access. Without reordering, the encoding would be limited to O(n) bulk decoding (similar to huffman encoding) due to the variable-sized keys. We did explore some indexes (they should be explained in the initial PR description) that could be used as a view for interfacing with the original order, so you get the benefit of reordering for random access while allowing access through the original ordering.
Do you think it's worth implementing these indexes as an option for the encoding?
…to align with C20
|
Hi @David-C-L, Thank you! The PR is getting into a better shape! can we update the decode time and memory overhead as well along with the compression ratio in the same benchmark which you have used in the description? Thank you |
| void FrequencyPartitionEncoding<T>::readWithVisitor( | ||
| V& visitor, | ||
| ReadWithVisitorParams& params) { | ||
| detail::readWithVisitorSlow(visitor, params, nullptr, [&] { |
There was a problem hiding this comment.
This is fine for initial implementation, but we can try or will follow up if needed for readWithVisitorFast
| } | ||
|
|
||
| template <typename T> | ||
| uint32_t FrequencyPartitionEncoding<T>::getTierForRow(uint32_t rowIndex) const { |
There was a problem hiding this comment.
We can try some search optimizations here if possible.
…etween the index types
…inal data order: PerTierBitmaps, TierTagArray, EliasFano
…itmaps, TierTagArray, EliasFano
TPCH SF10 Compression Rates per Column
Moderate number of unique int32 values following Zipfian distribution with varying alpha
Moderate number of unique int32 values -- Order-preserving index overhead
Comprehensive Encode, Decode, Compression, Memory use for Frequency Partition and FOR on simple Zipfian data
This PR also contains some changes for compatibility with Clang++-16.