Skip to content

Comments

Fix errors in the nvCOMP adapter#19221

Merged
rapids-bot[bot] merged 15 commits intorapidsai:branch-25.08from
vuule:fea-nvcomp-adapter-changes-pt2
Jul 17, 2025
Merged

Fix errors in the nvCOMP adapter#19221
rapids-bot[bot] merged 15 commits intorapidsai:branch-25.08from
vuule:fea-nvcomp-adapter-changes-pt2

Conversation

@vuule
Copy link
Contributor

@vuule vuule commented Jun 25, 2025

Description

Fix errors related to API changes (originally made in #18931).

Checklist

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

@copy-pr-bot
Copy link

copy-pr-bot bot commented Jun 25, 2025

Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually.

Contributors can view more details about this message here.

@github-actions github-actions bot added the libcudf Affects libcudf (C++/CUDA) code. label Jun 25, 2025
@vuule vuule added bug Something isn't working non-breaking Non-breaking change labels Jun 25, 2025
@mhaseeb123 mhaseeb123 self-requested a review June 26, 2025 19:15
@GregoryKimball GregoryKimball moved this to Burndown in libcudf Jul 11, 2025
@copy-pr-bot
Copy link

copy-pr-bot bot commented Jul 14, 2025

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@rapidsai rapidsai deleted a comment from copy-pr-bot bot Jul 15, 2025
@rapidsai rapidsai deleted a comment from copy-pr-bot bot Jul 15, 2025
@vuule vuule marked this pull request as ready for review July 15, 2025 15:42
@vuule vuule requested a review from a team as a code owner July 15, 2025 15:42
@vuule vuule requested a review from nvdbaranec July 15, 2025 15:42
capped_uncomp_bytes, nvcompBatchedSnappyCompressDefaultOpts, &max_comp_chunk_size);
break;
case compression_type::DEFLATE:
case compression_type::GZIP: // HACK!
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like we use deflate for GZIP. Can we write a more descriptive comment than “HACK”? Why was this done? Is there a TODO component? A missing API? An unsupported feature? A bug workaround?

Copy link
Contributor Author

@vuule vuule Jul 16, 2025

Choose a reason for hiding this comment

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

that comment (HACK) is a bit harsh :D
I can definitely replace with a better comment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, these should not yield the same result, so the HACK comment is on point.
Corrected the implementation.

status = nvcompBatchedSnappyCompressGetMaxOutputChunkSize(
capped_uncomp_bytes, nvcompBatchedSnappyCompressDefaultOpts, &max_comp_chunk_size);
break;
case compression_type::DEFLATE:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
case compression_type::DEFLATE:
case compression_type::DEFLATE: [[fallthrough]];

The fallthrough attribute helps tell the compiler and linters that this was intentional.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We should not use [[fallthrough]] here. The code guidelines only recommend its use for non-empty fall-through cases. I think that compiler also only warns in this case as well.

@vuule
Copy link
Contributor Author

vuule commented Jul 16, 2025

/ok to test 9e2cf07

@vuule
Copy link
Contributor Author

vuule commented Jul 17, 2025

/merge

@rapids-bot rapids-bot bot merged commit 9563317 into rapidsai:branch-25.08 Jul 17, 2025
169 of 171 checks passed
@vuule vuule deleted the fea-nvcomp-adapter-changes-pt2 branch July 17, 2025 16:09
@GregoryKimball GregoryKimball moved this from Burndown to Landed in libcudf Jul 17, 2025
galipremsagar pushed a commit to galipremsagar/cudf that referenced this pull request Jul 17, 2025
@GregoryKimball GregoryKimball removed this from libcudf Sep 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants