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 regression HostColumnVectorCore requiring native libs #9948

Merged
merged 1 commit into from Jan 5, 2022

Conversation

jlowe
Copy link
Member

@jlowe jlowe commented Dec 22, 2021

#9485 regressed the fix in #9332. This restores the fix. It also eliminates the problematic ColumnVector.closeBuffers which, despite the plurality in the name, only closed one buffer and had dubious utility in converting throwables into runtime exceptions.

@jlowe jlowe added bug Something isn't working cuDF (Java) Affects Java cuDF API. 4 - Needs cuDF (Java) Reviewer non-breaking Non-breaking change labels Dec 22, 2021
@jlowe jlowe requested a review from a team as a code owner December 22, 2021 15:50
@jlowe jlowe self-assigned this Dec 22, 2021
@jlowe jlowe added this to PR-WIP in v22.02 Release via automation Dec 22, 2021
v22.02 Release automation moved this from PR-WIP to PR-Reviewer approved Dec 22, 2021
Copy link
Contributor

@abellina abellina left a comment

Choose a reason for hiding this comment

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

LGTM

@jlowe
Copy link
Member Author

jlowe commented Jan 5, 2022

build

@jlowe
Copy link
Member Author

jlowe commented Jan 5, 2022

rerun tests

@codecov
Copy link

codecov bot commented Jan 5, 2022

Codecov Report

Merging #9948 (0cbdbdf) into branch-22.02 (967a333) will decrease coverage by 0.06%.
The diff coverage is n/a.

Impacted file tree graph

@@               Coverage Diff                @@
##           branch-22.02    #9948      +/-   ##
================================================
- Coverage         10.49%   10.42%   -0.07%     
================================================
  Files               119      119              
  Lines             20305    20475     +170     
================================================
+ Hits               2130     2134       +4     
- Misses            18175    18341     +166     
Impacted Files Coverage Δ
python/dask_cudf/dask_cudf/sorting.py 92.30% <0.00%> (-0.61%) ⬇️
python/cudf/cudf/__init__.py 0.00% <0.00%> (ø)
python/cudf/cudf/core/frame.py 0.00% <0.00%> (ø)
python/cudf/cudf/core/index.py 0.00% <0.00%> (ø)
python/cudf/cudf/io/parquet.py 0.00% <0.00%> (ø)
python/cudf/cudf/core/series.py 0.00% <0.00%> (ø)
python/cudf/cudf/utils/utils.py 0.00% <0.00%> (ø)
python/cudf/cudf/utils/dtypes.py 0.00% <0.00%> (ø)
python/cudf/cudf/utils/ioutils.py 0.00% <0.00%> (ø)
python/cudf/cudf/core/dataframe.py 0.00% <0.00%> (ø)
... and 15 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 1e74c2a...0cbdbdf. Read the comment docs.

Comment on lines +597 to +605
if (data != null) {
data.close();
}
if (offsets != null) {
offsets.close();
}
if (valid != null) {
valid.close();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

We could benefit from some 3rd-party or our own flavor of API callable like this: IOUtils.close(data, offsets, valid);

@jlowe
Copy link
Member Author

jlowe commented Jan 5, 2022

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 33f7f0d into rapidsai:branch-22.02 Jan 5, 2022
v22.02 Release automation moved this from PR-Reviewer approved to Done Jan 5, 2022
@vyasr vyasr added 4 - Needs Review Waiting for reviewer to review or respond and removed 4 - Needs cuDF (Java) Reviewer labels Feb 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4 - Needs Review Waiting for reviewer to review or respond bug Something isn't working cuDF (Java) Affects Java cuDF API. non-breaking Non-breaking change
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

5 participants