Skip to content

Use public Arrow functions for TDigest in PercentileApproxInputTypesTests#19648

Merged
rapids-bot[bot] merged 4 commits intorapidsai:branch-25.10from
davidwendt:tdigest-compute
Aug 12, 2025
Merged

Use public Arrow functions for TDigest in PercentileApproxInputTypesTests#19648
rapids-bot[bot] merged 4 commits intorapidsai:branch-25.10from
davidwendt:tdigest-compute

Conversation

@davidwendt
Copy link
Contributor

@davidwendt davidwendt commented Aug 11, 2025

Description

Changes the PercentileApproxInputTypesTests logic in percentile_approx_test.cpp to use the public arrow functions to compute the tdigest values instead of internal or detail functions.
This required enabling the ARROW_COMPUTE=ON functions to be enabled in the get_arrow.cmake to build the libarrow.so.

Checklist

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

@davidwendt davidwendt self-assigned this Aug 11, 2025
@davidwendt davidwendt added 2 - In Progress Currently a work in progress libcudf Affects libcudf (C++/CUDA) code. improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Aug 11, 2025
@copy-pr-bot
Copy link

copy-pr-bot bot commented Aug 11, 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.

@davidwendt
Copy link
Contributor Author

davidwendt commented Aug 11, 2025

/ok to test

@davidwendt
Copy link
Contributor Author

davidwendt commented Aug 11, 2025

/ok to test

bdice
bdice reviewed Aug 11, 2025
View reviewed changes
@davidwendt
Copy link
Contributor Author

davidwendt commented Aug 11, 2025

/ok to test

@vyasr
Copy link
Contributor

vyasr commented Aug 11, 2025

5f16c57 is not what you want to do. We do want those exports, the issue is that we don't build the parquet bits by default and the export set was placed inside the if(ENABLE_PARQUET) conditional. Revert https://nvidia.slack.com/archives/D02D7HWN823/p1752017801355159 and move that code outside of that conditional and it should get past the error in https://github.com/rapidsai/cudf/actions/runs/16884957641/job/47831783653.

@davidwendt davidwendt added 3 - Ready for Review Ready for review by team and removed 2 - In Progress Currently a work in progress labels Aug 11, 2025
@davidwendt davidwendt marked this pull request as ready for review August 11, 2025 20:38
@davidwendt davidwendt requested review from a team as code owners August 11, 2025 20:38
@davidwendt davidwendt requested review from bdice and mhaseeb123 August 11, 2025 20:38
vyasr
vyasr approved these changes Aug 11, 2025
View reviewed changes
Copy link
Contributor

@vyasr vyasr left a comment

Choose a reason for hiding this comment

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

I don't think that the CMake is quite right in not exporting the new targets, but I'm guessing it doesn't matter because we are only using this functionality in tests and because Spark is statically linking and doesn't need to propagate transitive requirements in the case of finding a preexisting Arrow lib, so I'm approving the current state as good enough to unblock us. If we eventually run into the limitations of it we can revisit improving the CMake.

Thanks David!

@vyasr vyasr mentioned this pull request Aug 11, 2025
Merged
3 tasks
bdice
bdice approved these changes Aug 11, 2025
View reviewed changes
Copy link
Contributor

@bdice bdice left a comment

Choose a reason for hiding this comment

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

Echoing Vyas's comments -- otherwise LGTM, thank you.

@davidwendt
Copy link
Contributor Author

davidwendt commented Aug 12, 2025

/merge

@rapids-bot rapids-bot bot merged commit bf63a7d into rapidsai:branch-25.10 Aug 12, 2025
127 of 138 checks passed
@davidwendt davidwendt deleted the tdigest-compute branch August 12, 2025 14:37
@Matt711 Matt711 mentioned this pull request Aug 12, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vyasr vyasr vyasr approved these changes

@bdice bdice bdice approved these changes

@mhaseeb123 mhaseeb123 mhaseeb123 approved these changes

Assignees

@davidwendt davidwendt

Labels

3 - Ready for Review Ready for review by team improvement Improvement / enhancement to an existing function libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@davidwendt @vyasr @bdice @mhaseeb123