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
+111 −10

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 requested review from vuule and ttnghia October 30, 2024 18:24
@mhaseeb123 mhaseeb123 requested a review from davidwendt October 31, 2024 01:36
@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 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
binmahone added a commit to binmahone/cudf that referenced this pull request Dec 30, 2024
@binmahone
Revert "Mark column chunks in a PQ reader pass as large strings whe…
5265bba 
…n the cumulative `offsets` exceeds the large strings threshold. (rapidsai#17207)"

This reverts commit e52ce85.
@binmahone
Copy link

binmahone commented Jan 2, 2025 •
edited
Loading

hi @mhaseeb123 , I have a silly question (because I'm not very familiar with CUDF code): why are cumulative string offsets counted per pass instead of per subpass? We have a parquet file whose string column in one RG alone is more than 2G, after #17207 such column will be treated as large string column and thus become unacceptable in Spark-Rapids (Spark-Rapids does not allow large columns, as described in #16215, and AFAIK this issue is not easy to fix)

If cumulative string offsets are counted per subpass, then we can keep using normal string columns in many cases. Is there any particular reason stopping us from going with subpass?

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
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
5 participants
@mhaseeb123 @binmahone @ttnghia @vuule @davidwendt