Fix the row index stream order in ORC reader #13242
Conversation
vuule
added
bug
github-actions
bot
added
Python
vuule
changed the title
| * | ||
| * @return The order of index streams | ||
| */ | ||
| static auto __device__ index_order_from_index_types(uint32_t index_types_bitmap) |
There was a problem hiding this comment.
Technically all free functions are static.
There was a problem hiding this comment.
Maybe I'm misunderstanding the comment, but static is used here to give the function internal linkage - it is not called outside of this translation unit. Maybe this is not relevant for __device__ functions, but we use this extensively in cuIO.
There was a problem hiding this comment.
Perhaps @ttnghia is confusing the concept of static member variables with usage of static as a storage class specifier? They are two completely different concepts that unfortunately reuse the same keyword. Otherwise I am not sure what the comment is referring to.
There was a problem hiding this comment.
but static is used here to give the function internal linkage
This makes sense. Otherwise, I don't see why it needs to be static. Sorry for the confusion.
Typically we can just put the function into an anonymous namespace for that purpose.
| * | ||
| * @return The order of index streams | ||
| */ | ||
| static auto __device__ index_order_from_index_types(uint32_t index_types_bitmap) |
There was a problem hiding this comment.
Perhaps @ttnghia is confusing the concept of static member variables with usage of static as a storage class specifier? They are two completely different concepts that unfortunately reuse the same keyword. Otherwise I am not sure what the comment is referring to.
|
/merge |
Description
Fixes #11890
Use fixed order when reading row index streams.
The order is
PRESENT,DATA,SECONDARY/LENGTH(maps toDATA2).Is any of these is absent in the column, relative order is maintained. Thus, the order is a sub-array of the one above.
Also simplified some logic related to stream order, as we do not need to pass it from the host. Instead, we only pass a bitmap to denote which streams are present.
Updated the xfail test, as it now passes :)
Checklist