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

[BUG] cudf::ast Module Fails Implicit Cast from Decimal to Double Due to Recent Explicit Conversion Requirement (PR #15438) #16023

Closed
aocsa opened this issue Jun 13, 2024 · 4 comments · Fixed by #16038 or #16045
Labels
bug Something isn't working libcudf Affects libcudf (C++/CUDA) code. tests Unit testing for project

Comments

@aocsa
Copy link
Contributor

aocsa commented Jun 13, 2024

Describe the bug

A recent pull request (PR) #15438 introduced a breaking change requiring that Floating <--> Fixed-Point conversions must be called explicitly, thereby disallowing implicit casts from decimal to double. Consequently, the cudf::ast module, which still employs implicit casting, is affected.

While cuDF version 24.06 has incorporated these changes into various cuDF functions, the necessary updates to the cudf::ast module were missed. This oversight results in the failure of implicit casts from decimal to double within the cudf::ast module, as highlighted in the following code:

return static_cast<To>(f);

Steps/Code to reproduce bug

TEST_F(TransformTest, BasicAdditionDoubleCast)
{
  auto c_0   = column_wrapper<double>{3, 20, 1, 50};
  //auto c_1   = column_wrapper<int8_t>{10, 7, 20, 0};
  std::vector<__int128_t> data1{10, 7, 20, 0};  
  auto c_1 = cudf::test::fixed_point_column_wrapper<__int128_t>(
                            data1.begin(), data1.end(), numeric::scale_type{0});
  auto table = cudf::table_view{{c_0, c_1}};
  auto col_ref_0  = cudf::ast::column_reference(0);
  auto col_ref_1  = cudf::ast::column_reference(1);
  auto cast       = cudf::ast::operation(cudf::ast::ast_operator::CAST_TO_FLOAT64, col_ref_1);
  auto expression = cudf::ast::operation(cudf::ast::ast_operator::ADD, col_ref_0, cast);
  auto expected = column_wrapper<double>{13, 27, 21, 50};
  auto result   = cudf::compute_column(table, expression);
  CUDF_TEST_EXPECT_COLUMNS_EQUAL(expected, result->view(), verbosity);
}

This unit test results in an exception:

C++ exception with description "CUDF failure at:/opt/mambaforge/conda-bld/libcudf-ext_1717805640036/work/cpp/include/cudf/ast/detail/operators.hpp:1063: Invalid unary operation." thrown in the test body.

Expected behavior

This test is expected to pass as it functions correctly in cuDF version 24.04.

Environment: conda
Method of cuDF install: source code
v24.06.00 branch release

@aocsa aocsa added the bug Something isn't working label Jun 13, 2024
@bdice
Copy link
Contributor

bdice commented Jun 13, 2024

@aocsa Thank you for the issue report!

@pmattione-nvidia Would you be able to take a look at this? I'm happy to show you around the AST codebase if you would like.

@Matt711 Matt711 added tests Unit testing for project libcudf Affects libcudf (C++/CUDA) code. labels Jun 14, 2024
rapids-bot bot pushed a commit that referenced this issue 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](#16023)

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

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

URL: #16038
@github-project-automation github-project-automation bot moved this from In Progress to Done in cuDF/Dask/Numba/UCX Jun 14, 2024
@bdice bdice reopened this Jun 15, 2024
@bdice
Copy link
Contributor

bdice commented Jun 15, 2024

I realized that we missed adding a test case for this in #16038. cc: @pmattione-nvidia

@vyasr vyasr mentioned this issue Jun 24, 2024
3 tasks
@cryos
Copy link
Contributor

cryos commented Jun 25, 2024

Could this fix be considered for inclusion in a 24.06 hotfix release?

rapids-bot bot pushed a commit that referenced this issue Jun 25, 2024
Add test for AST cast-to-float64

Resolves #16023

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

Approvers:
  - Vyas Ramasubramani (https://github.com/vyasr)
  - Bradley Dice (https://github.com/bdice)

URL: #16045
@vyasr
Copy link
Contributor

vyasr commented Jun 26, 2024

Yup, we are currently discussing what will be in our hotfix and the fixes for this issue are on the list.

vyasr pushed a commit to vyasr/cudf that referenced this issue 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 issue 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 issue Jun 26, 2024
Add test for AST cast-to-float64

Resolves rapidsai#16023

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

Approvers:
  - Vyas Ramasubramani (https://github.com/vyasr)
  - Bradley Dice (https://github.com/bdice)

URL: rapidsai#16045
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. tests Unit testing for project
Projects
Archived in project
5 participants