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

Add a shortcut for when the input clusters are all empty for the tdigest merge #16897

Merged
merged 8 commits into from
Oct 1, 2024
Merged

Add a shortcut for when the input clusters are all empty for the tdigest merge #16897

merged 8 commits into from
Oct 1, 2024

Conversation

jihoonson
Copy link
Contributor

@jihoonson jihoonson commented Sep 24, 2024 •
edited
Loading

Description

Fixes #16881. This is a new attempt to fix it.

Previously in #16675, I flipped the has_nulls flag to true as I thought that empty clusters should be explicitly stored in the offsets and handled properly. It turns out that it was not a good idea. After a long debugging process, I am convinced now that the existing logic is valid and should work well except for one case, where all input tdigests to the tdigest merge are empty. So, I have decided to add a shortcut to handle that particular edge case in group_merge_tdigest() in this PR. This shortcut is executed only when all clusters are empty in all groups. This PR does not change any other logic. Other changes in this PR are:

  • New unit tests to cover the edge case.
  • make_empty_tdigest_column has been renamed to make_tdigest_column_of_empty_clusters and expanded to take num_rows.
  • Some new documentation based on my understanding for the merge_tdigests() function.

Before making this PR, I have run the integration tests of the spark-rapids that were previously reported in NVIDIA/spark-rapids#11463 that my first attempt had caused them failing. They have all passed with this PR change.

Checklist

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

@jihoonson jihoonson requested a review from a team as a code owner September 24, 2024 20:19
@copy-pr-bot copy-pr-bot
Copy link

copy-pr-bot bot commented Sep 24, 2024

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.

@github-actions github-actions bot added the libcudf Affects libcudf (C++/CUDA) code. label Sep 24, 2024
@jihoonson
Copy link
Contributor Author

jihoonson commented Sep 24, 2024 •
edited
Loading

I have just pushed another commit that has only comments and docs change. The actual code change should be found only in the first commit.

@vyasr
Copy link
Contributor

vyasr commented Sep 25, 2024

Can you please retarget to and rebase on branch-24.12? We are currently in burndown, which is when new PRs should typically target the new release branch.

@jihoonson jihoonson changed the base branch from branch-24.10 to branch-24.12 September 25, 2024 16:31
@jihoonson
Copy link
Contributor Author

@vyasr thanks. I have updated the base branch for this PR. I'm trying now to build this PR on branch-24.12 locally to make sure everything is OK.

@jihoonson
Copy link
Contributor Author

@vyasr I've confirmed that this PR on branch-24.12 is passing all tdigest tests in GROUPBY_TEST and all tests in QUENTILEs_TEST. I have rebased my branch onto branch-24.12 now.

@PointKernel
Copy link
Member

/ok to test

@PointKernel PointKernel added bug Something isn't working 3 - Ready for Review Ready for review by team non-breaking Non-breaking change labels Sep 25, 2024
nvdbaranec
nvdbaranec requested changes Sep 25, 2024
View reviewed changes
cpp/src/quantiles/tdigest/tdigest_aggregation.cu Show resolved Hide resolved
cpp/src/quantiles/tdigest/tdigest_aggregation.cu Outdated Show resolved Hide resolved
cpp/src/quantiles/tdigest/tdigest_aggregation.cu Outdated Show resolved Hide resolved
cpp/src/quantiles/tdigest/tdigest_aggregation.cu Outdated Show resolved Hide resolved
cpp/src/quantiles/tdigest/tdigest_aggregation.cu Outdated Show resolved Hide resolved
cpp/src/quantiles/tdigest/tdigest_aggregation.cu Outdated Show resolved Hide resolved
@PointKernel
Copy link
Member

/ok to test

@jihoonson
Copy link
Contributor Author

/merge

1 similar comment
@ttnghia
Copy link
Contributor

ttnghia commented Oct 1, 2024

/merge

@rapids-bot rapids-bot bot merged commit 194d5f4 into rapidsai:branch-24.12 Oct 1, 2024
100 checks passed
@PointKernel
Copy link
Member

The PR was merged while I was still reviewing it. Please note that two C++ approvals are required before merging any libcudf PR.

@jihoonson
Copy link
Contributor Author

@PointKernel thanks, that is good to know. Please let me know if you have any comment. I will address them in a follow-up PR.

@jihoonson jihoonson mentioned this pull request Oct 2, 2024
Closed
@davidwendt davidwendt mentioned this pull request Oct 23, 2024
Merged
3 tasks
rapids-bot bot pushed a commit that referenced this pull request Oct 24, 2024
@davidwendt
Remove unused variable in internal merge_tdigests utility (#17151)
b75036b 
Removes unused variable that contains host copy of the group_offsets data.
This host variable appears to have been made obsolete by a combination of #16897 and #16780
Found while working on #17149

Authors:
  - David Wendt (https://github.com/davidwendt)

Approvers:
  - Muhammad Haseeb (https://github.com/mhaseeb123)
  - Nghia Truong (https://github.com/ttnghia)

URL: #17151
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PointKernel PointKernel PointKernel left review comments

@nvdbaranec nvdbaranec nvdbaranec approved these changes

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

Assignees
No one assigned
Labels
3 - Ready for Review Ready for review by team bug Something isn't working libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change
Projects
None yet
Milestone
No milestone
5 participants
@jihoonson @vyasr @PointKernel @ttnghia @nvdbaranec