Fix reading Parquet string cols when nrows and input_pass_limit > 0
#17321
Conversation
github-actions
bot
added
libcudf
mhaseeb123
added
bug
| @pytest.mark.parametrize("pass_read_limit", [0, 240, 1024000000]) | ||
| @pytest.mark.parametrize("use_pandas_metadata", [True, False]) | ||
| @pytest.mark.parametrize("num_rows", [1000, 3000, None]) | ||
| def test_parquet_chunked_reader_structs( |
There was a problem hiding this comment.
Similar test as above (test_parquet_chunked_reader) but with a struct table.
cpp/src/io/parquet/page_decode.cuh
Outdated
| return has_repetition | ||
| ? ((page_begin <= begin && page_end >= begin) || (page_begin <= end && page_end >= end)) | ||
| : ((page_begin < begin && page_end > begin) || (page_begin < end && page_end > end)); | ||
| : ((page_begin < begin && page_end > begin) || (page_begin < end && page_end >= end)); |
There was a problem hiding this comment.
cudf/cpp/src/io/parquet/reader_impl_preprocess.cu
Lines 1373 to 1376 in 9da8eb2
^Here we adjust the num_rows for the last page when input_pass_limit > 0 to compensate for lists. This leads to a false negative is_bounds_page for the last page of str cols due to the page_end > end predicate. This false negative results in page_bounds() including total size of last page instead of clipping it at nrows. This finally manifests in the out_buf.create_string_data() with larger num_bytes than data size leading to the extra \x00 chars in the column.
There was a problem hiding this comment.
@nvdbaranec I didn't see the effect of adjusting num_rows for the last page showing up for other col types and is_bounds_page is only used in string decoders so I am hoping other decoders don't rely on a similar predicate (at least doesn't seem like in the tests).
There was a problem hiding this comment.
You know it's a convoluted bug when the fix is 99% comment.
Should we try to keep the condition and change thenum_rows adjustment? This fix feels like a workaround for incorrect num_rows (expecting to be wrong on this one, but want to make sure we at least consider this).
There was a problem hiding this comment.
Ikr, I have been debating myself with this one as well. I am not fully sure if we would break things by adjusting num_rows for the last pages of cols (to what?) in generate_list_column_row_count_estimates() and keep the original condition here. Maybe @nvdbaranec or @etseidl can comment here better.
For now, I chose to relax this condition as this function (is_bounds_page()) is only being used in string decoders (narrower scope) and the described problem is only being seen in string cols.
There was a problem hiding this comment.
This makes me nervous. So in the normal case, won't this mean every final page is a bounds page, regardless of whether we're using num_rows? That means unnecessary processing when all we want to do is read the entire page anyway. Could we add a boolean argument to is_bounds_page if num_rows has been modified, or add a field to the PageInfo struct? Or maybe do as @vuule suggests (change the num_rows adjustment)...perhaps add an adjusted_num_rows 🤷
There was a problem hiding this comment.
Thanks for the feedback, Ed! I have added a new boolean field to PageInfo which tells if the num_rows for this page has been adjusted and if so, we relax the condition.
cpp/src/io/parquet/page_decode.cuh
Outdated
| return has_repetition | ||
| ? ((page_begin <= begin && page_end >= begin) || (page_begin <= end && page_end >= end)) | ||
| : ((page_begin < begin && page_end > begin) || (page_begin < end && page_end > end)); | ||
| : ((page_begin < begin && page_end > begin) || (page_begin < end && page_end >= end)); |
There was a problem hiding this comment.
You know it's a convoluted bug when the fix is 99% comment.
Should we try to keep the condition and change thenum_rows adjustment? This fix feels like a workaround for incorrect num_rows (expecting to be wrong on this one, but want to make sure we at least consider this).
…ero-nrows-and-input-pass-limit
Co-authored-by: Vukasin Milovanovic <[email protected]>
…ero-nrows-and-input-pass-limit
…ero-nrows-and-input-pass-limit
mhaseeb123
added
5 - Ready to Merge
…ero-nrows-and-input-pass-limit
|
@mhaseeb123 Can we go ahead and merge this pr? |
|
/merge |
mhaseeb123
deleted the
bug/pq-chunk-read-str-cols-with-nonzero-nrows-and-input-pass-limit
branch
Description
This PR fixes reading string columns in Parquet using chunked parquet reader when
nrowsandinput_pass_limitare > 0.Closes #17311
Checklist