Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Mark column chunks in a PQ reader pass as large strings when the cumulative offsets exceeds the large strings threshold. #17207

Merged
merged 21 commits into from
Nov 7, 2024
Merged

Mark column chunks in a PQ reader pass as large strings when the cumulative offsets exceeds the large strings threshold. #17207

merged 21 commits into from
Nov 7, 2024

Conversation

mhaseeb123
Copy link
Member

@mhaseeb123 mhaseeb123 commented Oct 30, 2024 •
edited
Loading

Description

This PR implements a method to correctly set the large-string property for column chunks in a in the Chunked Parquet Reader subpass if the cumulative string offsets have exceeded the large strings threshold.

Fixes #17158

Checklist

  • I am familiar with the Contributing Guidelines.
  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.

@mhaseeb123 mhaseeb123 self-assigned this Oct 30, 2024
@github-actions github-actions bot added the libcudf Affects libcudf (C++/CUDA) code. label Oct 30, 2024
@mhaseeb123 mhaseeb123 added 2 - In Progress Currently a work in progress bug Something isn't working non-breaking Non-breaking change and removed libcudf Affects libcudf (C++/CUDA) code. labels Oct 30, 2024
@github-actions github-actions bot added the libcudf Affects libcudf (C++/CUDA) code. label Oct 30, 2024
@mhaseeb123 mhaseeb123 changed the title 🚧 Correctly set the large-string property of (non-large string) column chunks if previously seen to be large-string columns. 🚧 Correctly set (non-large) column chunks to be large-string if part of a previously large-string column. Oct 30, 2024
@mhaseeb123 mhaseeb123 marked this pull request as ready for review October 31, 2024 01:36
@mhaseeb123 mhaseeb123 requested a review from a team as a code owner October 31, 2024 01:37
@mhaseeb123 mhaseeb123 changed the title 🚧 Correctly set (non-large) column chunks to be large-string if part of a previously large-string column. Correctly set (non-large) column chunks to be large-string if part of a previously large-string column. Oct 31, 2024
mhaseeb123
mhaseeb123 commented Oct 31, 2024
View reviewed changes
cpp/tests/large_strings/parquet_tests.cpp

int constexpr multiplier = 12;
std::vector<cudf::column_view> input_cols(multiplier, input->view());
auto col0 = cudf::concatenate(input_cols); ///< 2.70GB
Copy link
Member Author

@mhaseeb123 mhaseeb123 Oct 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same column from GTest: CaseTest.ToLower

@mhaseeb123 mhaseeb123 changed the title Correctly set (non-large) column chunks to be large-string if part of a previously large-string column. Correctly set large string column chunks in a PQ reader pass if cumulative string offsets have exceeded the large string threshold. Oct 31, 2024
@mhaseeb123 mhaseeb123 changed the title Correctly set large string column chunks in a PQ reader pass if cumulative string offsets have exceeded the large string threshold. Correctly mark column chunks in a PQ reader pass as large strings when the cumulative offsets exceeds the large strings threshold. Oct 31, 2024
@mhaseeb123 mhaseeb123 changed the title Correctly mark column chunks in a PQ reader pass as large strings when the cumulative offsets exceeds the large strings threshold. Mark column chunks in a PQ reader pass as large strings when the cumulative offsets exceeds the large strings threshold. Oct 31, 2024
mhaseeb123 and others added 3 commits October 31, 2024 11:44
@mhaseeb123 @davidwendt
Apply suggestion
b0c033d 
Co-authored-by: David Wendt <45795991+davidwendt@users.noreply.github.com>
@mhaseeb123
minor style fix
9b9273e
@mhaseeb123
minor
ab64a46
@mhaseeb123 mhaseeb123 added 3 - Ready for Review Ready for review by team and removed 2 - In Progress Currently a work in progress labels Oct 31, 2024
@mhaseeb123 mhaseeb123 requested a review from vuule November 4, 2024 18:56
@mhaseeb123 mhaseeb123 added 4 - Needs Review Waiting for reviewer to review or respond and removed 3 - Ready for Review Ready for review by team labels Nov 4, 2024
@vuule vuule requested a review from davidwendt November 4, 2024 19:28
vuule
vuule reviewed Nov 4, 2024
View reviewed changes
Copy link
Contributor

@vuule vuule left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

small comments; the core of the change looks good.

cpp/src/io/parquet/reader_impl.cpp Show resolved Hide resolved
cpp/tests/large_strings/parquet_tests.cpp Outdated Show resolved Hide resolved
cpp/tests/large_strings/parquet_tests.cpp Outdated Show resolved Hide resolved
cpp/tests/large_strings/parquet_tests.cpp Show resolved Hide resolved
vuule
vuule approved these changes Nov 5, 2024
View reviewed changes
Copy link
Contributor

@vuule vuule left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for iterating on this!

@mhaseeb123 mhaseeb123 added 5 - Ready to Merge Testing and reviews complete, ready to merge and removed 4 - Needs Review Waiting for reviewer to review or respond labels Nov 6, 2024
@mhaseeb123
Copy link
Member Author

/merge

@rapids-bot rapids-bot bot merged commit e52ce85 into rapidsai:branch-24.12 Nov 7, 2024
102 checks passed
@mhaseeb123 mhaseeb123 deleted the bug/chunked-pq-reader-invalid-large-string-cols branch November 8, 2024 06:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davidwendt davidwendt davidwendt approved these changes

@vuule vuule vuule approved these changes

@ttnghia ttnghia ttnghia approved these changes

Assignees

@mhaseeb123 mhaseeb123

Labels
5 - Ready to Merge Testing and reviews complete, ready to merge bug Something isn't working libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change
Projects
cuDF/Dask/Numba/UCX
Status: Done
libcudf
Status: Landed
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[BUG] Chunked parquet reader incorrect results for large string columns
4 participants
@mhaseeb123 @ttnghia @vuule @davidwendt