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

Fix cudf::merge gtest for dictionary columns #6942

Merged
merged 2 commits into from
Dec 9, 2020

Conversation

davidwendt
Copy link
Contributor

The cudf::merge API expects the key columns to be sorted. This means that if null rows are included, these null entries should all appear either at beginning or at the end of the column depending on the null_order for the sort. The MergeDictionaryTest.WithNull gtest placed null rows in the middle of the column. The expected results should also have included null entries at the beginning or the end.

This PR also includes an extra test for checking merge results are consistent with the sort parameters cudf::order and cudf::null_order. This test also includes a larger number of rows to ensure thrust::merge requires more than one tile/block in its runtime logic.

@davidwendt davidwendt requested a review from a team as a code owner December 8, 2020 16:20
@davidwendt davidwendt self-assigned this Dec 8, 2020
@davidwendt davidwendt added 3 - Ready for Review Ready for review by team libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change labels Dec 8, 2020
@GPUtester
Copy link
Collaborator

Please update the changelog in order to start CI tests.

View the gpuCI docs here.

@davidwendt davidwendt added this to PR-WIP in v0.18 Release via automation Dec 8, 2020
@davidwendt davidwendt moved this from PR-WIP to PR-Needs review in v0.18 Release Dec 8, 2020
@davidwendt davidwendt added the bug Something isn't working label Dec 8, 2020
v0.18 Release automation moved this from PR-Needs review to PR-Reviewer approved Dec 8, 2020
@codecov
Copy link

codecov bot commented Dec 8, 2020

Codecov Report

Merging #6942 (dc97685) into branch-0.18 (917759b) will increase coverage by 0.46%.
The diff coverage is n/a.

Impacted file tree graph

@@               Coverage Diff               @@
##           branch-0.18    #6942      +/-   ##
===============================================
+ Coverage        81.57%   82.03%   +0.46%     
===============================================
  Files               96       96              
  Lines            15912    16282     +370     
===============================================
+ Hits             12980    13357     +377     
+ Misses            2932     2925       -7     
Impacted Files Coverage Δ
python/cudf/cudf/io/feather.py 100.00% <0.00%> (ø)
python/cudf/cudf/comm/serialize.py 0.00% <0.00%> (ø)
python/cudf/cudf/_fuzz_testing/io.py 0.00% <0.00%> (ø)
python/dask_cudf/dask_cudf/_version.py 0.00% <0.00%> (ø)
python/dask_cudf/dask_cudf/io/tests/test_csv.py 100.00% <0.00%> (ø)
python/dask_cudf/dask_cudf/io/tests/test_orc.py 100.00% <0.00%> (ø)
python/dask_cudf/dask_cudf/io/tests/test_json.py 100.00% <0.00%> (ø)
...ython/dask_cudf/dask_cudf/io/tests/test_parquet.py 100.00% <0.00%> (ø)
python/cudf/cudf/core/dataframe.py 91.08% <0.00%> (+<0.01%) ⬆️
python/cudf/cudf/utils/applyutils.py 98.74% <0.00%> (+0.02%) ⬆️
... and 36 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 917759b...dc97685. Read the comment docs.

@harrism harrism added 6 - Okay to Auto-Merge tests Unit testing for project and removed 3 - Ready for Review Ready for review by team labels Dec 9, 2020
@rapids-bot rapids-bot bot merged commit b45fd4d into rapidsai:branch-0.18 Dec 9, 2020
v0.18 Release automation moved this from PR-Reviewer approved to Done Dec 9, 2020
@davidwendt davidwendt deleted the fix-merge-test branch November 8, 2021 14:47
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 tests Unit testing for project
Projects
No open projects
v0.18 Release
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

5 participants