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

Update tests/column_utilities to use experimental::equality row comparator #12777

Merged

Conversation

divyegala
Copy link
Member

This PR is a part of #11844

Checklist

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

@divyegala divyegala added feature request New feature or request 5 - DO NOT MERGE Hold off on merging; see PR for details non-breaking Non-breaking change labels Feb 14, 2023
@github-actions github-actions bot added the libcudf Affects libcudf (C++/CUDA) code. label Feb 14, 2023
@divyegala divyegala removed the 5 - DO NOT MERGE Hold off on merging; see PR for details label Mar 8, 2023
@divyegala divyegala marked this pull request as ready for review March 8, 2023 12:27
@divyegala divyegala requested a review from ttnghia March 16, 2023 18:16
@divyegala
Copy link
Member Author

@ttnghia all the mixed type test failures are now solved, these are the only failing tests. Any advice would be appreciated:

[ RUN      ] FixedPointTestAllReps/0.SimpleFixedPointColumnWrapper
unknown file: Failure
C++ exception with description "CUDF failure at: /opt/conda/conda-bld/work/cpp/src/table/row_operators.cu:337: Cannot compare tables with different column types" thrown in the test body.

[ RUN      ] StringsConvertTest.ToFixedPointLargeScale
unknown file: Failure
C++ exception with description "CUDF failure at: /opt/conda/conda-bld/work/cpp/src/table/row_operators.cu:337: Cannot compare tables with different column types" thrown in the test body.

[ RUN      ] JsonPathTests.GetJsonObjectEmptyQuery
unknown file: Failure
C++ exception with description "CUDF failure at: /opt/conda/conda-bld/work/cpp/src/strings/strings_column_view.cpp:31: strings column has no children" thrown in the test body.

[ RUN      ] StringsSplitTest.AllNullsCase
unknown file: Failure
C++ exception with description "CUDF failure at: /opt/conda/conda-bld/work/cpp/src/strings/strings_column_view.cpp:31: strings column has no children" thrown in the test body.

@divyegala
Copy link
Member Author

@davidwendt according to your baseline report, column_utilities.cu takes around ~14 minutes to build. In this PR, it takes around ~10 minutes https://downloads.rapids.ai/ci/cudf/pull-request/12777/121e45b/cpp_cuda11_x86_64.BuildMetricsReport.html

@ttnghia
Copy link
Contributor

ttnghia commented Mar 21, 2023

The first two failed tests compare decimals with different scales. I'm not sure if we should continue to support it or not (for the new comparator). Please initialize a discussion on Slack to get more feedback for it.

The last two failed tests are due to comparing strings in a strings column having all-nulls (which doesn't have a child column). I believe this is a bug. So you need to file a bug issue for it, then either fix that bug before this PR can proceed, or xfail these two tests.

@davidwendt
Copy link
Contributor

@davidwendt according to your baseline report, column_utilities.cu takes around ~14 minutes to build. In this PR, it takes around ~10 minutes https://downloads.rapids.ai/ci/cudf/pull-request/12777/121e45b/cpp_cuda11_x86_64.BuildMetricsReport.html

Thanks. I'd still like to consider this change: #12777 (comment)

@davidwendt
Copy link
Contributor

This PR uncovers a bug in this code:

bool has_nonempty_null_rows(cudf::column_view const& input, rmm::cuda_stream_view stream)
{
if (not input.has_nulls()) { return false; } // No nulls => no dirty rows.
// Cross-reference nullmask and offsets.
auto const type = input.type().id();
auto const offsets = (type == type_id::STRING) ? (strings_column_view{input}).offsets()
: (lists_column_view{input}).offsets();
auto const d_input = cudf::column_device_view::create(input, stream);
auto const is_dirty_row = [d_input = *d_input, offsets = offsets.begin<size_type>()] __device__(
size_type const& row_idx) {
return d_input.is_null_nocheck(row_idx) && (offsets[row_idx] != offsets[row_idx + 1]);
};
auto const row_begin = thrust::counting_iterator<cudf::size_type>(0);
auto const row_end = row_begin + input.size();
return thrust::count_if(rmm::exec_policy(stream), row_begin, row_end, is_dirty_row) > 0;
}

Accessing data at null rows is considered undefined behavior which this function does intentionally. Both an all-null strings column and an empty strings column may not have children. I believe that is the same case for list columns though it may actually have an empty child column simply to identify its type.

I propose this function could be fixed to handle this correctly in this PR by adding the following if-statement near the top:

if ((input.size() == input.null_count()) && (input.num_children() == 0)) { return false; }

@davidwendt
Copy link
Contributor

davidwendt commented Mar 21, 2023

For the fixed-point error, it appears that the intention was to be able to compare fixed-point values in different scales. The FixedPointTestAllReps/SimpleFixedPointColumnWrapper gtests specifically tests this.
I propose to relax the check here:

bool column_types_equal(column_view const& lhs, column_view const& rhs)
{
if (lhs.type() != rhs.type()) { return false; }
return type_dispatcher(lhs.type(), columns_equal_fn{}, lhs, rhs);
}

To only check the type id by changing the if-statement there to:

if (lhs.type().id() != rhs.type().id()) { return false; }

EDIT: Looks like that causes another test to fail at ColumnTypeCheckTest.DifferentFixedWidth so it looks like this fix will need to be further up the chain where this is called. Perhaps a new utility like this is needed:

bool column_types_comparable(column_view const& lhs, column_view const& rhs)
{
  if (lhs.type().id() != rhs.type().id()) { return false; }
  return type_dispatcher(lhs.type(), columns_equal_fn{}, lhs, rhs);
}

Then the check_shape_compatibility in row_operators.cu should be changed to call this new function.
Maybe there is a better word than comparable here.

@@ -147,6 +147,7 @@ TEST_F(ColumnTypeCheckTest, DifferentFixedWidth)
fixed_point_column_wrapper<int32_t> rhs5({10000}, numeric::scale_type{0});

EXPECT_FALSE(column_types_equal(lhs5, rhs5));
EXPECT_TRUE(column_types_equivalent(lhs5, rhs5));
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice.

@divyegala divyegala requested a review from ttnghia March 21, 2023 21:20
@divyegala
Copy link
Member Author

/merge

@rapids-bot rapids-bot bot merged commit 913302a into rapidsai:branch-23.04 Mar 22, 2023
rapids-bot bot pushed a commit that referenced this pull request Mar 24, 2023
Fixes a bug introduced in #12777 (by me) in `column_utilities.cu` that caused the difference reporting in a failed comparison in a gtest result (i.e. through `CUDF_TEST_EXPECT_COLUMNS_EQUIVALENT` for example) to report the incorrect row.
The logic was changed to improve compile time but incorrectly created indices of only 0s and 1s in the difference vector.
This PR fixes the logic to create the correct indices for the reporting logic.

Authors:
  - David Wendt (https://github.com/davidwendt)

Approvers:
  - Nghia Truong (https://github.com/ttnghia)
  - Divye Gala (https://github.com/divyegala)

URL: #12995
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request New feature or request libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants