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] copy_if_else can throw for nested types corner case #8322

Closed
revans2 opened this issue May 21, 2021 · 1 comment · Fixed by #8359
Closed

[BUG] copy_if_else can throw for nested types corner case #8322

revans2 opened this issue May 21, 2021 · 1 comment · Fixed by #8359
Labels
bug Something isn't working libcudf Affects libcudf (C++/CUDA) code.

Comments

@revans2
Copy link
Contributor

revans2 commented May 21, 2021

Describe the bug
Because of the bug fixed by #8314 I decided to take a look at everywhere that make_empty_column is used without an explicit type id.

if (boolean_mask.is_empty()) { return cudf::make_empty_column(lhs.type()); }

Is one of those places.

This happens before any type dispatch so if you passed in an empty mask instead of getting an empty column back you get an exception.

I also noticed that the type check is only for a single level type id. So it verifies that both are list or struct columns, but it does not verify that the child types are the same all the way down. This means if we fix the main issue with this bug we will end up with another issue where we might get an empty column that matches the lhs, but if lhs does not match rhs it will not be an exception.

Steps/Code to reproduce bug

diff --git a/cpp/tests/copying/copy_if_else_nested_tests.cpp b/cpp/tests/copying/copy_if_else_nested_tests.cpp
index 9ac34a3044..ea9c6462a9 100644
--- a/cpp/tests/copying/copy_if_else_nested_tests.cpp
+++ b/cpp/tests/copying/copy_if_else_nested_tests.cpp
@@ -135,6 +135,29 @@ TYPED_TEST(TypedCopyIfElseNestedTest, Lists)
   CUDF_TEST_EXPECT_COLUMNS_EQUIVALENT(result_column->view(), expected_output->view());
 }
 
+TYPED_TEST(TypedCopyIfElseNestedTest, ListsEmptyMask)
+{
+  using T = TypeParam;
+
+  using namespace cudf;
+  using namespace cudf::test;
+
+  using lcw = lists_column_wrapper<T, int32_t>;
+
+  auto lhs = lcw{}.release();
+  auto rhs = lcw{}.release();
+
+  auto selector_column = fixed_width_column_wrapper<bool, int32_t>{}.release();
+
+  auto result_column = copy_if_else(lhs->view(), rhs->view(), selector_column->view());
+
+  auto expected_output = lcw{}.release();
+
+  CUDF_TEST_EXPECT_COLUMNS_EQUIVALENT(result_column->view(), expected_output->view());
+}
+
+
+
 TYPED_TEST(TypedCopyIfElseNestedTest, ListsWithNulls)
 {
   using T = TypeParam;
@revans2 revans2 added bug Something isn't working Needs Triage Need team to review and classify labels May 21, 2021
@kkraus14 kkraus14 added libcudf Affects libcudf (C++/CUDA) code. and removed Needs Triage Need team to review and classify labels May 24, 2021
@nvdbaranec
Copy link
Contributor

So, this is not a hard fix, but it's also not simple because we're dealing with both columns and scalars here. If it were just columns we could call empty_like() and that would probably be it. But we have no scalar -> column empty_like(). I can add this, but it feels a little late to be doing it (13 hours to code freeze).

rapids-bot bot pushed a commit that referenced this issue May 27, 2021
Fixes:  #8322

The code was calling `make_empty_column()` to produce empty results, which does not work for nested types. Fix was to use `empty_like()` instead.

As part of this, I implemented a new public function:

`std::unique_ptr<column> empty_like(scalar const& input);`

Authors:
  - https://github.com/nvdbaranec

Approvers:
  - Jake Hemstad (https://github.com/jrhemstad)
  - Nghia Truong (https://github.com/ttnghia)
  - Paul Taylor (https://github.com/trxcllnt)
  - MithunR (https://github.com/mythrocks)

URL: #8359
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.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants