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 producing incorrect answers for structs #8356

Closed
revans2 opened this issue May 25, 2021 · 3 comments · Fixed by #8507
Closed

[BUG] copy_if_else producing incorrect answers for structs #8356

revans2 opened this issue May 25, 2021 · 3 comments · Fixed by #8507
Assignees
Labels
bug Something isn't working libcudf Affects libcudf (C++/CUDA) code. Spark Functionality that helps Spark RAPIDS

Comments

@revans2
Copy link
Contributor

revans2 commented May 25, 2021

Describe the bug
When trying to implement Case/When and If/Else for Spark for Structs I ran into a test that was failing. I brought the size of the test data down a lot, but it looks like NULLs are being inserted into incorrect places for some result/

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..7ad374ca3b 100644
--- a/cpp/tests/copying/copy_if_else_nested_tests.cpp
+++ b/cpp/tests/copying/copy_if_else_nested_tests.cpp
@@ -102,6 +102,66 @@ TYPED_TEST(TypedCopyIfElseNestedTest, StructsWithNulls)
   CUDF_TEST_EXPECT_COLUMNS_EQUIVALENT(result_column->view(), expected_result->view());
 }
 
+TYPED_TEST(TypedCopyIfElseNestedTest, LongerStructsWithNulls)
+{
+  using T = TypeParam;
+
+  using namespace cudf;
+  using namespace cudf::test;
+
+  using ints    = fixed_width_column_wrapper<T, int32_t>;
+  using structs = structs_column_wrapper;
+  using bools   = fixed_width_column_wrapper<bool, int32_t>;
+
+  auto selector_column = bools{
+      1, 1, 1, 1, 0, 1, 0, 1, 0, 0,
+      1, 0, 0, 0, 1, 0, 0, 1, 0, 1,
+      0, 0, 0, 0, 1, 1, 1, 1, 0, 0,
+      1, 0, 1, 1, 0, 0, 1, 0, 0, 1,
+      0, 0, 1, 0, 0, 1, 0, 0, 0, 1,
+      0, 0, 1, 0, 1, 0, 1, 1, 1, 1,
+      1, 0, 1, 1, 1, 0}.release();
+
+  std::cerr << "AT 1" << std::endl;
+  auto lhs_child_0 = ints{{
+       120,   16,   53, -121,  -15,   21,  -33,   72,   54,   90,
+       -67, -117,  -77,    0,  113,   15,  -87,   77, -128,    0,
+         0,    0,  115,   49, -107,   10,  -62,  -78,  -49,   73,
+        92,  -22,    0,  127,  -49,  101,  -82,    0,    6, -110,
+        69,  -29,  -63,  105,  -18,   84, -113,  -65,    0,   29,
+       -61,   53,   62,   87,  -84,  -49, -108,  -85,  -21,   21,
+        84,   53,   56,  -56,  -53,  -28
+      }, iterator_with_null_at(std::vector<size_type>{13, 19, 20, 21, 32, 37, 48})};
+
+  std::cerr << "AT 2" << std::endl;
+  auto lhs_child_1 = ints{{
+        27,  -80,  -24,   76,  -56,   42,    5,   13,  -69,  -77,
+        61,  -77,   72,    0,   31,  118,  -30,   86,  125,    0,
+         0,    0,   75,  -49,  125,   60,  116,  118,   64,   20,
+       -70,  -18,    0,  -25,   22,  -46,  -89,   -9,   27,  -56,
+       -77,  123,    0,  -90,   87, -113,  -37,   22,  -22,  -53,
+        73,   99,  113,   -2,  -24,  113,   75,    6,   82,  -58,
+       122, -123, -127,   19,  -62,  -24
+      }, iterator_with_null_at(std::vector<size_type>{13, 19, 20, 21, 32, 42})};
+
+  std::cerr << "AT 3" << std::endl;
+  auto lhs_structs_column = structs{{lhs_child_0, lhs_child_1}, 
+      iterator_with_null_at(std::vector<size_type>{13, 19, 20, 21, 32})}.release();
+
+  std::cerr << "AT 4" << std::endl;
+  cudf::test::print(*lhs_structs_column);
+
+  auto result_column =
+    copy_if_else(lhs_structs_column->view(), lhs_structs_column->view(), selector_column->view());
+
+  std::cerr << "AT 8" << std::endl;
+  cudf::test::print(*result_column);
+
+  CUDF_TEST_EXPECT_COLUMNS_EQUIVALENT(result_column->view(), lhs_structs_column->view());
+}
+
+
+
 TYPED_TEST(TypedCopyIfElseNestedTest, Lists)
 {
   using T = TypeParam;

Expected behavior
The test should pass because the the lhs and rhs of the copy_if_else is the same thing so I don't see how we would ever get anything back except the lhs, but we get back a different answer (and some how it also crashes, which is separate)

@revans2 revans2 added bug Something isn't working Needs Triage Need team to review and classify Spark Functionality that helps Spark RAPIDS labels May 25, 2021
@shwina shwina removed the Needs Triage Need team to review and classify label May 26, 2021
@harrism harrism added the libcudf Affects libcudf (C++/CUDA) code. label May 27, 2021
@gerashegalov gerashegalov self-assigned this Jun 1, 2021
@gerashegalov
Copy link
Contributor

a smaller repro:

diff --git a/cpp/tests/copying/copy_if_else_nested_tests.cpp b/cpp/tests/copying/copy_if_else_nested_tests.cpp
index 9ac34a3044..0f7fc625e4 100644
--- a/cpp/tests/copying/copy_if_else_nested_tests.cpp
+++ b/cpp/tests/copying/copy_if_else_nested_tests.cpp
@@ -102,6 +102,34 @@ TYPED_TEST(TypedCopyIfElseNestedTest, StructsWithNulls)
   CUDF_TEST_EXPECT_COLUMNS_EQUIVALENT(result_column->view(), expected_result->view());
 }
 
+TYPED_TEST(TypedCopyIfElseNestedTest, ReflectiveStructsWithNulls)
+{
+  using T = TypeParam;
+
+  using namespace cudf;
+  using namespace cudf::test;
+
+  using ints    = fixed_width_column_wrapper<T, int32_t>;
+  using structs = structs_column_wrapper;
+  using bools   = fixed_width_column_wrapper<bool, int32_t>;
+
+  auto selector_column = bools{0, 1}.release();
+
+  auto lhs_child_0 = ints{{100, 101}, iterator_with_null_at(std::vector<size_type>{1})};
+
+  auto lhs_structs_column = structs{{lhs_child_0}}.release();
+
+  cudf::test::print(*lhs_structs_column);
+
+  auto result_column =
+    copy_if_else(lhs_structs_column->view(), lhs_structs_column->view(), selector_column->view());
+
+  std::cout << "result: " << std::endl;
+  cudf::test::print(*result_column);
+
+  CUDF_TEST_EXPECT_COLUMNS_EQUIVALENT(result_column->view(), lhs_structs_column->view());
+}
+
 TYPED_TEST(TypedCopyIfElseNestedTest, Lists)
 {
   using T = TypeParam;

@gerashegalov
Copy link
Contributor

As for SIGSEGV:

Thread 1 "COPYING_TEST" received signal SIGSEGV, Segmentation fault.
0x0000555556b76a48 in cudf::test::(anonymous namespace)::stringify_column_differences[abi:cxx11](cudf::device_span<int const, 18446744073709551615ul>, cudf::column_view const&, cudf::column_view const&, bool, int) ()

@gerashegalov
Copy link
Contributor

gerashegalov added a commit to gerashegalov/cudf that referenced this issue Jun 12, 2021
Simplifies copy_if_else by skipping gather on lhs, and using lhs directly
as the rhs scatter destinations for 0-rows in select col.

Closes rapidsai#8356

Co-authored-by: MithunR <[email protected]>
rapids-bot bot pushed a commit that referenced this issue Jun 14, 2021
Fixes the scatter map size to the target column size. 
Simplifies `copy_if_else` by skipping gather on lhs, and using lhs directly as the rhs scatter destinations for 0-rows in select col.

Closes #8356

Co-authored-by: @mythrocks

Authors:
  - Gera Shegalov (https://github.com/gerashegalov)

Approvers:
  - Nghia Truong (https://github.com/ttnghia)
  - Mark Harris (https://github.com/harrism)
  - MithunR (https://github.com/mythrocks)

URL: #8507
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. Spark Functionality that helps Spark RAPIDS
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants