Implement equality of two table_views - #22319
Conversation
At various times, it is useful to check whether two tables are equal. For example, in cudf-polars we use this to check if two tables are "compatibly" partitioned. Previously there have been no such utilities in libcudf proper. The best one can do is to loop over the columns, call cudf::binary_operation with NULL_EQUALS and then cudf::reduce on the result. This launches many more kernels than necessary. Instead, use the existing row_equality operators to perform a single transform_reduce over the table checking for equality.
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
The row_operator function is too complex for transform-reduce, resulting in very long compile times in general, and a bug in cicc 13.1.
Previously if two tables had column types that were not equality-comparable, cudf::logic_error was thrown, while the documented exception was std::invalid_argument. Fix this by throwing the correct exception.
davidwendt
left a comment
There was a problem hiding this comment.
New APIs should be added a streams test as well
https://github.com/rapidsai/cudf/tree/main/cpp/tests/streams
This is just to ensure the stream is passed all the way through the underlying CUDA calls.
No need to test all variations of parameters and no need to verify the results since a stream violation will surface as an exception.
Should we consider a benchmark for this API as well? Though I would not deem it necessary to merge this.
Thanks. Done. |
ttnghia
left a comment
There was a problem hiding this comment.
Should the test utility cudf::test::detail::expect_tables_equal modified to adopt this?
I think no, because that test equality has special casing for floating point (you can compare to some ulp) and it would have to run over the whole tables again anyway to generate the differences when things are not equal? |
Perhaps this can be done in a follow-on PR. Maybe that is what you are suggesting since you already approved this one. |
Yup, just as a note for follow up. I can open a PR as soon as this one merges |
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR adds cudf::tables_equal: a CUDA-accelerated table-view equality API and implementation, strengthens exception handling to use std::invalid_argument for non-comparable types, adds build wiring for the new source and stream test, and expands unit tests covering value/shape/type/null/NaN/nested cases. ChangesTable Equality Function
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
cpp/include/cudf/table/equality.hpp (1)
30-33: ⚡ Quick winAdd
[[nodiscard]]attribute.The function is side-effect-free and returns a meaningful
boolvalue that should not be discarded. As per coding guidelines, side-effect-free functions with non-void return types should have the[[nodiscard]]attribute.🔧 Suggested fix
-bool tables_equal(table_view const& left, - table_view const& right, - null_equality nulls_equal = null_equality::EQUAL, - rmm::cuda_stream_view stream = cudf::get_default_stream()); +[[nodiscard]] bool tables_equal(table_view const& left, + table_view const& right, + null_equality nulls_equal = null_equality::EQUAL, + rmm::cuda_stream_view stream = cudf::get_default_stream());As per coding guidelines: "Add [[nodiscard]] attribute to side-effect-free functions with non-void return types"
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cpp/include/cudf/table/equality.hpp` around lines 30 - 33, The tables_equal declaration is a pure query whose boolean result must not be ignored; add the [[nodiscard]] attribute to its declaration (i.e., change the signature to [[nodiscard]] bool tables_equal(...)) and mirror the attribute on any corresponding definition/overload for consistency (ensure the attribute is placed immediately before the return type in the declaration/definition of tables_equal).cpp/tests/streams/table_equality_test.cpp (1)
17-26: 💤 Low valueCapture the return value to avoid potential warnings.
While stream tests primarily validate stream propagation rather than functional correctness, the return value should still be captured to avoid potential unused-result warnings.
📝 Suggested improvement
TEST_F(TableEqualTest, NotEqual) { cudf::test::fixed_width_column_wrapper<int> left( {{0, 0, 0, 0, 0}, {false, false, true, true, true}}); cudf::test::fixed_width_column_wrapper<int> right({1, 1, 1, 1, 1}); - cudf::tables_equal(cudf::table_view{{left}}, - cudf::table_view{{right}}, - cudf::null_equality::EQUAL, - cudf::test::get_default_stream()); + [[maybe_unused]] auto result = cudf::tables_equal(cudf::table_view{{left}}, + cudf::table_view{{right}}, + cudf::null_equality::EQUAL, + cudf::test::get_default_stream()); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cpp/tests/streams/table_equality_test.cpp` around lines 17 - 26, The test TEST_F(TableEqualTest, NotEqual) calls cudf::tables_equal but ignores its return value, which can trigger unused-result warnings; modify the test to capture the result (e.g., auto result = cudf::tables_equal(...) or bool equal = cudf::tables_equal(...)) and, if desired, assert on it (ASSERT_FALSE/ASSERT_TRUE) or cast to void to silence warnings—update the call site where cudf::tables_equal(cudf::table_view{{left}}, cudf::table_view{{right}}, cudf::null_equality::EQUAL, cudf::test::get_default_stream()) is invoked.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@cpp/include/cudf/table/equality.hpp`:
- Around line 30-33: The tables_equal declaration is a pure query whose boolean
result must not be ignored; add the [[nodiscard]] attribute to its declaration
(i.e., change the signature to [[nodiscard]] bool tables_equal(...)) and mirror
the attribute on any corresponding definition/overload for consistency (ensure
the attribute is placed immediately before the return type in the
declaration/definition of tables_equal).
In `@cpp/tests/streams/table_equality_test.cpp`:
- Around line 17-26: The test TEST_F(TableEqualTest, NotEqual) calls
cudf::tables_equal but ignores its return value, which can trigger unused-result
warnings; modify the test to capture the result (e.g., auto result =
cudf::tables_equal(...) or bool equal = cudf::tables_equal(...)) and, if
desired, assert on it (ASSERT_FALSE/ASSERT_TRUE) or cast to void to silence
warnings—update the call site where cudf::tables_equal(cudf::table_view{{left}},
cudf::table_view{{right}}, cudf::null_equality::EQUAL,
cudf::test::get_default_stream()) is invoked.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 1639e346-5f79-4afd-9049-ae7ac4110d43
📒 Files selected for processing (8)
cpp/CMakeLists.txtcpp/include/cudf/table/equality.hppcpp/src/row_operator/row_operators.cucpp/src/table/table_equal.cucpp/tests/CMakeLists.txtcpp/tests/streams/table_equality_test.cppcpp/tests/table/table_tests.cppcpp/tests/types/traits_test.cpp
e20feb7 to
12f428c
Compare
12f428c to
d612665
Compare
|
/merge |
At various times, it is useful to check whether two tables are equal. For example, in cudf-polars we use this to check if two tables are "compatibly" partitioned. Previously there have been no such utilities in libcudf proper. The best one can do is to loop over the columns, call cudf::binary_operation with NULL_EQUALS and then cudf::reduce on the result. This launches many more kernels than necessary. Instead, use the existing row_equality operators to perform a single transform_reduce over the table checking for equality. Authors: - Lawrence Mitchell (https://github.com/wence-) - Muhammad Haseeb (https://github.com/mhaseeb123) Approvers: - David Wendt (https://github.com/davidwendt) - Muhammad Haseeb (https://github.com/mhaseeb123) URL: NVIDIA#22319
At various times, it is useful to check whether two tables are equal. For example, in cudf-polars we use this to check if two tables are "compatibly" partitioned. Previously there have been no such utilities in libcudf proper. The best one can do is to loop over the columns, call cudf::binary_operation with NULL_EQUALS and then cudf::reduce on the result. This launches many more kernels than necessary. Instead, use the existing row_equality operators to perform a single transform_reduce over the table checking for equality. Authors: - Lawrence Mitchell (https://github.com/wence-) - Muhammad Haseeb (https://github.com/mhaseeb123) Approvers: - David Wendt (https://github.com/davidwendt) - Muhammad Haseeb (https://github.com/mhaseeb123) URL: NVIDIA#22319
Follow up #22319 Use `cudf::tables_equal` API in libcudf examples instead of anti join to check if two tables are equal Authors: - Muhammad Haseeb (https://github.com/mhaseeb123) - Lawrence Mitchell (https://github.com/wence-) - Vukasin Milovanovic (https://github.com/vuule) Approvers: - Shruti Shivakumar (https://github.com/shrshi) - David Wendt (https://github.com/davidwendt) URL: #22428
xref #22319 Hoping to use this in rapidsmpf for Python testing (Primarily agent generated) Authors: - Matthew Roeschke (https://github.com/mroeschke) Approvers: - Bradley Dice (https://github.com/bdice) - Muhammad Haseeb (https://github.com/mhaseeb123) URL: #22611
Description
At various times, it is useful to check whether two tables are equal. For example, in cudf-polars we use this to check if two tables are "compatibly" partitioned. Previously there have been no such utilities in libcudf proper. The best one can do is to loop over the columns, call cudf::binary_operation with NULL_EQUALS and then cudf::reduce on the result. This launches many more kernels than necessary.
Instead, use the existing row_equality operators to perform a single transform_reduce over the table checking for equality.
Checklist