Sort dictionary data alphabetically in the ORC writer #14295
Merged
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
vuule
added
cuIO
vuule
commented
Oct 17, 2023
Comment on lines
+873
to
+881
| auto const is_str_dict = | ||
| ck.type_kind == TypeKind::STRING and ck.encoding_kind == DICTIONARY_V2; | ||
| ck.dict_index = is_str_dict ? column.host_stripe_dict(stripe.id).index.data() : nullptr; | ||
| ck.dict_data_order = | ||
| is_str_dict ? column.host_stripe_dict(stripe.id).data_order.data() : nullptr; | ||
| ck.dtype_len = (ck.type_kind == TypeKind::STRING) ? 1 : column.type_width(); | ||
| ck.scale = column.scale(); | ||
| ck.decimal_offsets = | ||
| (ck.type_kind == TypeKind::DECIMAL) ? column.decimal_offsets() : nullptr; |
There was a problem hiding this comment.
Some of these were left uninitialized when unused, changed to always initialize.
…bug-sort-orc-dict
…o bug-sort-orc-dict
|
For my internal test, our diff vs the CPU went from 22% to 5%, which is really impressive. Thanks for working on this.
Do you expect a 3% slow down to the write because of the sort for dictionary encoded data? |
The slowdown is up to 22% unfortunately. Sorting is not cheap :( |
davidwendt
reviewed
Oct 23, 2023
davidwendt
reviewed
Oct 25, 2023
…bug-sort-orc-dict
davidwendt
approved these changes
Oct 25, 2023
divyegala
approved these changes
Oct 25, 2023
cpp/src/io/orc/writer_impl.cu
Outdated
| stripe_dicts.host_to_device_async(stream); | ||
|
|
||
| // Sort stripe dictionaries alphabetically | ||
| auto streams = cudf::detail::fork_streams(stream, std::min<size_t>(dict_order_owner.size(), 8)); |
There was a problem hiding this comment.
Is 8 streams an empirical choice?
There was a problem hiding this comment.
it is
I tried powers of two up to 32 and 8 was the fastest one. There wasn't a big difference compared to other 4+ values, though.
|
Before we merge, mind if I run orc benchmarks on our stuff? I should be able to get these back to you tomorrow. @vuule |
vuule
added
5 - DO NOT MERGE
With the above we believe default=on makes sense but we really like having the flag you added @vuule, because it allows us to experiment easily and you never know what pathological cases we may run into. Thank you!! |
abellina
approved these changes
Oct 27, 2023
vuule
added
5 - Ready to Merge
|
/merge |
3 tasks
raydouglass
pushed a commit
that referenced
this pull request
Dec 8, 2023
… to ORC (#14595) Changes in #14295 introduced a synchronization issue in `build_dictionaries`. After stripe_dicts are initialized on the host, we copy them to the device and then launch kernels that read the dicts (device copy). However, after these kernels we deallocate buffers that are not longer needed and clear the dicts' views to these buffers on the host. The problem is that, without synchronization after the H2D copy, the host modification can be done before the H2D copy is performed, and we run the kernels with the altered state. This PR adds a sync point to make sure the copy is done before host-side modification. Authors: - Vukasin Milovanovic (https://github.com/vuule) Approvers: - Nghia Truong (https://github.com/ttnghia) - Alessandro Bellina (https://github.com/abellina) - Bradley Dice (https://github.com/bdice)
karthikeyann
pushed a commit
to karthikeyann/cudf
that referenced
this pull request
Dec 12, 2023
… to ORC (rapidsai#14595) Changes in rapidsai#14295 introduced a synchronization issue in `build_dictionaries`. After stripe_dicts are initialized on the host, we copy them to the device and then launch kernels that read the dicts (device copy). However, after these kernels we deallocate buffers that are not longer needed and clear the dicts' views to these buffers on the host. The problem is that, without synchronization after the H2D copy, the host modification can be done before the H2D copy is performed, and we run the kernels with the altered state. This PR adds a sync point to make sure the copy is done before host-side modification. Authors: - Vukasin Milovanovic (https://github.com/vuule) Approvers: - Nghia Truong (https://github.com/ttnghia) - Alessandro Bellina (https://github.com/abellina) - Bradley Dice (https://github.com/bdice)
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.
Description
Strings in the dictionary data streams are now sorted alphabetically.
Reduces file size in some cases because compression can be more efficient.
Reduces throughput up to 22% when writing strings columns (3% speedup when dictionary encoding is not used, though!).
Benchmark data does not demonstrate the compression difference, but we have some user data that compresses almost 30% better.
Checklist