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

Deprecate decimal_cols_as_float in ORC reader (C++ layer) #10152

Merged
merged 1 commit into from
Jan 28, 2022
Merged

Deprecate decimal_cols_as_float in ORC reader (C++ layer) #10152

merged 1 commit into from
Jan 28, 2022

Conversation

vuule
Copy link
Contributor

@vuule vuule commented Jan 27, 2022

Issue #10129

@vuule vuule added libcudf Affects libcudf (C++/CUDA) code. cuIO cuIO issue tech debt improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Jan 27, 2022
@vuule vuule self-assigned this Jan 27, 2022
@codecov
Copy link

codecov bot commented Jan 27, 2022 •
edited
Loading

Codecov Report

Merging #10152 (be5e354) into branch-22.04 (e24fa8f) will increase coverage by 0.05%.
The diff coverage is n/a.

Impacted file tree graph

@@               Coverage Diff                @@
##           branch-22.04   #10152      +/-   ##
================================================
+ Coverage         10.37%   10.43%   +0.05%     
================================================
  Files               119      119              
  Lines             20149    20594     +445     
================================================
+ Hits               2091     2148      +57     
- Misses            18058    18446     +388
Impacted Files Coverage Δ
python/cudf/cudf/errors.py 0.00% <0.00%> (ø)
python/cudf/cudf/io/csv.py 0.00% <0.00%> (ø)
python/cudf/cudf/io/hdf.py 0.00% <0.00%> (ø)
python/cudf/cudf/io/orc.py 0.00% <0.00%> (ø)
python/cudf/cudf/__init__.py 0.00% <0.00%> (ø)
python/cudf/cudf/_version.py 0.00% <0.00%> (ø)
python/cudf/cudf/core/abc.py 0.00% <0.00%> (ø)
python/cudf/cudf/api/types.py 0.00% <0.00%> (ø)
python/cudf/cudf/io/dlpack.py 0.00% <0.00%> (ø)
python/cudf/cudf/core/frame.py 0.00% <0.00%> (ø)
... and 60 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 5dd1c39...be5e354. Read the comment docs.

@vuule vuule marked this pull request as ready for review January 27, 2022 22:50
@vuule vuule requested a review from a team as a code owner January 27, 2022 22:50
@vuule vuule requested review from vyasr and devavret January 27, 2022 22:50
bdice
bdice reviewed Jan 27, 2022
View reviewed changes
cpp/include/cudf/io/orc.hpp
@@ -220,7 +220,9 @@ class orc_reader_options {
*
* @param val Vector of fully qualified column names.
*/
void set_decimal_cols_as_float(std::vector<std::string> val)
[[deprecated(
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this add a compiler warning when building libcudf?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It should issue warnings only when the function is used.
https://en.cppreference.com/w/cpp/language/attributes/deprecated

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. I read that page earlier but thought it was showing compiler output and not runtime output. Thanks for the correction.

vyasr
vyasr approved these changes Jan 27, 2022
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.

Should we also update the documentation to indicate that the method is deprecated? I don't think we have any real policies around this in libcudf, and I doubt many people actually read our doxygen, but it might still be nice to add a message. Otherwise LGTM.

@vuule
Copy link
Contributor Author

vuule commented Jan 28, 2022

Should we also update the documentation to indicate that the method is deprecated? I don't think we have any real policies around this in libcudf, and I doubt many people actually read our doxygen, but it might still be nice to add a message. Otherwise LGTM.

I haven't done so when previously deprecating APIs, but I'm not opposed to it. Do you know of an example where we included this info in the docs?

@vyasr
Copy link
Contributor

vyasr commented Jan 28, 2022

Should we also update the documentation to indicate that the method is deprecated? I don't think we have any real policies around this in libcudf, and I doubt many people actually read our doxygen, but it might still be nice to add a message. Otherwise LGTM.

I haven't done so when previously deprecating APIs, but I'm not opposed to it. Do you know of an example where we included this info in the docs?

I do not. It would be nice for us to have some policy around how deprecations should be handled in libcudf (we have a rough one in cuDF Python) but no need to hold up this PR for that if we don't already have a standard. It might be something we could define and document in the dev guide for future use, though.

@vuule
Copy link
Contributor Author

vuule commented Jan 28, 2022

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 5257f34 into rapidsai:branch-22.04 Jan 28, 2022
@vuule vuule deleted the deprecate-dec2float branch January 28, 2022 07:31
@vyasr vyasr mentioned this pull request Jan 28, 2022
Closed
@vyasr
Copy link
Contributor

vyasr commented Jan 28, 2022

@vuule I created #10159 to track this.

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

@bdice bdice bdice left review comments

@ttnghia ttnghia ttnghia approved these changes

@vyasr vyasr vyasr approved these changes

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

Assignees

@vuule vuule

Labels
cuIO cuIO issue 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
@vuule @vyasr @bdice @ttnghia