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 decimal -> float cast in ast code #16038

Merged
merged 3 commits into from
Jun 14, 2024

Conversation

pmattione-nvidia
Copy link
Contributor

@pmattione-nvidia pmattione-nvidia commented Jun 14, 2024

Fix decimal -> float cast in ast code that was missed during the earlier code refactoring for making the cast explicit.

This closes issue 16023

Checklist

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

@pmattione-nvidia pmattione-nvidia requested a review from a team as a code owner June 14, 2024 16:55
@pmattione-nvidia pmattione-nvidia self-assigned this Jun 14, 2024
@github-actions github-actions bot added the libcudf Affects libcudf (C++/CUDA) code. label Jun 14, 2024
@pmattione-nvidia pmattione-nvidia added improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Jun 14, 2024
Copy link
Member

@mhaseeb123 mhaseeb123 left a comment

Choose a reason for hiding this comment

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

LGTM. Just curious if we can reuse or consolidate the convert_fixed_to_floating at https://github.com/rapidsai/cudf/blob/branch-24.08/cpp/include/cudf/unary.hpp#L80 to avoid code duplication?

@pmattione-nvidia
Copy link
Contributor Author

LGTM. Just curious if we can reuse or consolidate the convert_fixed_to_floating at https://github.com/rapidsai/cudf/blob/branch-24.08/cpp/include/cudf/unary.hpp#L80 to avoid code duplication?

Yeah, that's what I wanted to do, but the code is relying on that decltype(static_cast(f)) to SFINAE-out would-be casts like string_view -> double that would otherwise cause compile-time errors.

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.

One small improvement request that I forgot to mention.

cpp/include/cudf/ast/detail/operators.hpp Outdated Show resolved Hide resolved
cpp/include/cudf/ast/detail/operators.hpp Show resolved Hide resolved
Co-authored-by: Bradley Dice <bdice@bradleydice.com>
@pmattione-nvidia
Copy link
Contributor Author

/merge

@rapids-bot rapids-bot bot merged commit 74b3826 into rapidsai:branch-24.08 Jun 14, 2024
73 checks passed
vyasr pushed a commit to vyasr/cudf that referenced this pull request Jun 26, 2024
Fix decimal -> float cast in ast code that was missed during the earlier code refactoring for making the cast explicit.  

This closes [issue 16023](rapidsai#16023)

Authors:
  - Paul Mattione (https://github.com/pmattione-nvidia)

Approvers:
  - Muhammad Haseeb (https://github.com/mhaseeb123)
  - Bradley Dice (https://github.com/bdice)

URL: rapidsai#16038
vyasr pushed a commit to vyasr/cudf that referenced this pull request Jun 26, 2024
Fix decimal -> float cast in ast code that was missed during the earlier code refactoring for making the cast explicit.  

This closes [issue 16023](rapidsai#16023)

Authors:
  - Paul Mattione (https://github.com/pmattione-nvidia)

Approvers:
  - Muhammad Haseeb (https://github.com/mhaseeb123)
  - Bradley Dice (https://github.com/bdice)

URL: rapidsai#16038
@raydouglass raydouglass mentioned this pull request Jul 1, 2024
3 tasks
raydouglass pushed a commit that referenced this pull request Jul 1, 2024
Backporting #16038 for a patch release.

---------

Co-authored-by: Paul Mattione <156858817+pmattione-nvidia@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement Improvement / enhancement to an existing function libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change
Projects
Status: Done
3 participants