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.

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

Add metadata_builder helper class #13232

Merged
merged 7 commits into from
May 1, 2023

Conversation

abellina
Copy link
Contributor

@abellina abellina commented Apr 26, 2023

This PR introduces a helper class metadata_builder within the cudf::detail namespace, that allows pack and contiguous_split to create metadata for a packed buffer without having access to metadata internals (e.g. serialized_column).

The class makes it possible for callers to build metadata independent of the current implementations in pack, that are relying on column_views being defined (actual valid cuDF columns). The chunked pack work will use this class to create metadata without needing to create a column_view in the first place, since it doesn't have a valid base pointer for the columns to be instantiated.

Signed-off-by: Alessandro Bellina <[email protected]>
@github-actions github-actions bot added the libcudf Affects libcudf (C++/CUDA) code. label Apr 26, 2023
@abellina abellina mentioned this pull request Apr 26, 2023
4 tasks
@abellina abellina added improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Apr 26, 2023
@abellina
Copy link
Contributor Author

I have tested with this patch in the past, and I know the cpp tests passed before I posted the PR. I had a linker error with pyarrow when it was trying to dlopen one of its internal libraries (so not even to the tests). As a result I have been recreating the conda environment and still waiting on that. Marking this as reviewable for now as I think getting some 👀 should be good and I don't expect new issues to show up in the python tests.

@abellina abellina marked this pull request as ready for review April 26, 2023 22:31
@abellina abellina requested a review from a team as a code owner April 26, 2023 22:31
@abellina
Copy link
Contributor Author

I have tested with this patch in the past, and I know the cpp tests passed before I posted the PR. I had a linker error with pyarrow when it was trying to dlopen one of its internal libraries (so not even to the tests). As a result I have been recreating the conda environment and still waiting on that. Marking this as reviewable for now as I think getting some eyes should be good and I don't expect new issues to show up in the python tests.

Local build finished and test_pack passed (CI also passed..)

cpp/src/copying/pack.cpp Outdated Show resolved Hide resolved
@abellina
Copy link
Contributor Author

Thanks @mythrocks, updated.

Copy link
Contributor

@mythrocks mythrocks left a comment

Choose a reason for hiding this comment

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

Sorry for the slow review. I'll look through this some more tomorrow.

cpp/include/cudf/detail/contiguous_split.hpp Show resolved Hide resolved
cpp/src/copying/pack.cpp Show resolved Hide resolved
cpp/src/copying/pack.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@hyperbolic2346 hyperbolic2346 left a comment

Choose a reason for hiding this comment

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

Mostly const additions requested.

cpp/include/cudf/detail/contiguous_split.hpp Show resolved Hide resolved
cpp/include/cudf/detail/contiguous_split.hpp Outdated Show resolved Hide resolved
cpp/include/cudf/detail/contiguous_split.hpp Outdated Show resolved Hide resolved
cpp/src/copying/pack.cpp Outdated Show resolved Hide resolved
cpp/src/copying/pack.cpp Outdated Show resolved Hide resolved
cpp/src/copying/pack.cpp Outdated Show resolved Hide resolved
cpp/src/copying/pack.cpp Outdated Show resolved Hide resolved
cpp/src/copying/pack.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@mythrocks mythrocks left a comment

Choose a reason for hiding this comment

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

LGTM!

@abellina
Copy link
Contributor Author

Thanks for the reviews so far @mythrocks and @hyperbolic2346. Let me know if there's anything else @hyperbolic2346

@abellina
Copy link
Contributor Author

abellina commented May 1, 2023

/merge

@rapids-bot rapids-bot bot merged commit f27be56 into rapidsai:branch-23.06 May 1, 2023
@abellina abellina deleted the pack_metadata_builder branch May 1, 2023 12:49
@abellina abellina mentioned this pull request May 1, 2023
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement Improvement / enhancement to an existing function libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants