-
Notifications
You must be signed in to change notification settings - Fork 1.1k
[BUG] Parquet reader can use an uninitialized required BINARY length under a null ancestor #23755
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
Changes from 4 commits
852ddba
c942ed0
4aa72b4
b6d3143
6a5503d
30fab0b
f464b0d
1b257bf
2d6d5bf
9a02e44
16c338e
2279895
39d1867
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1479,12 +1479,27 @@ __device__ void zero_fill_null_positions_shared( | |
|
|
||
| // nesting level that is storing actual leaf values | ||
| int const leaf_level_index = s->setup.col.max_nesting_depth - 1; | ||
| auto const& ni = s->nesting.nesting_info[leaf_level_index]; | ||
| auto const& leaf_ni = s->nesting.nesting_info[leaf_level_index]; | ||
|
|
||
| // A required Parquet leaf can be absent because one of its ancestors is optional. Since with RMM, | ||
| // the reader reader can leave the validity map associated w/the ancestor unwritten, this code | ||
| // zero-fills the gap rows by borrowing the nearest ancestor's validity bitmap instead. | ||
|
abigalekim marked this conversation as resolved.
Outdated
|
||
| auto const& ni = [&]() -> PageNestingDecodeInfo const& { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Great find. unfortunately more needs to be fixed: zero_fill_null_positions_shared() won't even be called from most of the call-sites in this scenario because it's using the wrong valid_map and computing the wrong num_values (e.g. the call in page_data.cu). Perhaps this logic needs to be factored out into a utility function so it can be used at those locations as well. |
||
| if (leaf_ni.valid_map != nullptr) { return leaf_ni; } | ||
| if (s->setup.col.max_level[level_type::REPETITION] != 0) { return leaf_ni; } | ||
| for (int idx = leaf_level_index - 1; idx >= 0; --idx) { | ||
| auto const& ancestor_ni = s->nesting.nesting_info[idx]; | ||
| if (ancestor_ni.valid_map != nullptr) { return ancestor_ni; } | ||
| } | ||
| return leaf_ni; | ||
| }(); | ||
|
|
||
| // Check if we have nulls to fill | ||
| if ((ni.valid_map == nullptr) || (num_values == 0)) { return; } | ||
|
|
||
| auto const data_out = ni.data_out; | ||
| if (&ni != &leaf_ni) { valid_map_offset = ni.valid_map_offset; } | ||
|
coderabbitai[bot] marked this conversation as resolved.
Outdated
abigalekim marked this conversation as resolved.
Outdated
|
||
|
|
||
| auto const data_out = leaf_ni.data_out; | ||
|
|
||
| constexpr int bits_per_mask = cudf::detail::size_in_bits<bitmask_type>(); | ||
| using cudf::detail::warp_size; | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.