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

Enable round-tripping of large strings in cudf #15944

Merged
merged 24 commits into from
Jun 12, 2024
Merged

Enable round-tripping of large strings in cudf #15944

merged 24 commits into from
Jun 12, 2024

Conversation

galipremsagar
Copy link
Contributor

@galipremsagar galipremsagar commented Jun 6, 2024 •
edited
Loading

Description

Fixes: #15922

This PR adds support for round-tripping LargeStringArray in cudf using 64 bit offsets.

Checklist

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

@galipremsagar galipremsagar requested a review from a team as a code owner June 6, 2024 18:11
@galipremsagar galipremsagar self-assigned this Jun 6, 2024
@github-actions github-actions bot added the libcudf Affects libcudf (C++/CUDA) code. label Jun 6, 2024
@galipremsagar galipremsagar added 2 - In Progress Currently a work in progress improvement Improvement / enhancement to an existing function non-breaking Non-breaking change and removed libcudf Affects libcudf (C++/CUDA) code. labels Jun 6, 2024
davidwendt
davidwendt requested changes Jun 6, 2024
View reviewed changes
Copy link
Contributor

@davidwendt davidwendt left a comment

Choose a reason for hiding this comment

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

I do not think wee need to check cudf::strings::detail::is_large_strings_enabled() when creating arrow LargeString objects.

cpp/src/interop/to_arrow.cu Outdated
} else {
return std::make_shared<arrow::StringArray>(
0, std::move(tmp_offset_buffer), std::move(tmp_data_buffer));
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This should not be needed. I see no reason to create a separate empty LargeStringArray.

cpp/src/interop/to_arrow.cu Outdated Show resolved Hide resolved
@github-actions github-actions bot added the libcudf Affects libcudf (C++/CUDA) code. label Jun 7, 2024
@galipremsagar galipremsagar requested a review from a team as a code owner June 10, 2024 14:29
@galipremsagar galipremsagar requested review from vyasr and Matt711 June 10, 2024 14:29
@github-actions github-actions bot added the Python Affects Python cuDF API. label Jun 10, 2024
@galipremsagar galipremsagar added 3 - Ready for Review Ready for review by team and removed 2 - In Progress Currently a work in progress labels Jun 10, 2024
@galipremsagar galipremsagar changed the title Enable round-tripping if large strings are enabled in cudf Enable round-tripping of large strings in cudf Jun 10, 2024
davidwendt
davidwendt requested changes Jun 10, 2024
View reviewed changes
Copy link
Contributor

@davidwendt davidwendt left a comment

Choose a reason for hiding this comment

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

Would it be possible to add gtests for this?

cpp/src/interop/from_arrow.cu Outdated
Comment on lines 304 to 305
auto chars_column = dispatch_to_cudf_column{}.operator()<int8_t>(
*char_array, data_type(type_id::INT8), true, stream, mr);
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs to be updated to use an rmm::device_buffer since making this a column will trigger the size_type limit.

wence-
wence- approved these changes Jun 11, 2024
View reviewed changes
Copy link
Contributor

@wence- wence- left a comment

Choose a reason for hiding this comment

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

Approving python changes, some tiny C++ suggestions.

cpp/src/interop/to_arrow.cu Outdated Show resolved Hide resolved
cpp/src/interop/from_arrow.cu Outdated Show resolved Hide resolved
cpp/src/interop/from_arrow.cu Outdated Show resolved Hide resolved
@galipremsagar
Copy link
Contributor Author

/merge

@galipremsagar galipremsagar added 5 - Ready to Merge Testing and reviews complete, ready to merge and removed 3 - Ready for Review Ready for review by team labels Jun 12, 2024
@rapids-bot rapids-bot bot merged commit e57f0fe into rapidsai:branch-24.08 Jun 12, 2024
74 checks passed
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

@bdice bdice bdice approved these changes

@wence- wence- wence- approved these changes

@srinivasyadav18 srinivasyadav18 Awaiting requested review from srinivasyadav18 srinivasyadav18 is a code owner automatically assigned from rapidsai/cudf-cpp-codeowners

@nvdbaranec nvdbaranec Awaiting requested review from nvdbaranec nvdbaranec is a code owner automatically assigned from rapidsai/cudf-cpp-codeowners

@vyasr vyasr Awaiting requested review from vyasr vyasr is a code owner automatically assigned from rapidsai/cudf-python-codeowners

@Matt711 Matt711 Awaiting requested review from Matt711 Matt711 is a code owner automatically assigned from rapidsai/cudf-python-codeowners

Assignees

@galipremsagar galipremsagar

Labels
5 - Ready to Merge Testing and reviews complete, ready to merge improvement Improvement / enhancement to an existing function libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change Python Affects Python cuDF API.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[FEA] Support large strings in to_pandas
4 participants
@galipremsagar @wence- @bdice @davidwendt