Two-table comparators with strong index types by bdice · Pull Request #10730 · rapidsai/cudf

bdice · 2022-04-25T19:36:52Z

This PR resolves #10508. It introduces two-table lexicographic row comparators with strongly typed index types. Given tables lhs and rhs, the two_table_comparator can create a device comparator whose strongly typed call operator can compare bidirectionally: lhs[i] < rhs[j] and rhs[i] < lhs[j]. The strong typing indicates which index belongs to which table.

This PR also contains a sample implementation in search_ordered.cu, which implements lower_bound and upper_bound algorithms.

cpp/include/cudf/detail/utilities/strong_index.hpp

ttnghia · 2022-05-02T22:26:04Z

FYI, here is my simple version of table_comarator classes without strong typed index:

cpp/include/cudf/table/experimental/row_operators.cuh

jrhemstad · 2022-05-04T22:55:48Z

cpp/include/cudf/table/experimental/row_operators.cuh

+  __device__ bool operator()(lhs_index_type const lhs_index,
+                             rhs_index_type const rhs_index) const noexcept


I think we'll need overloads for (lhs_index_type, lhs_index_type) and (rhs_index_type, rhs_index_type) as well.

Possible — but I haven’t found an algorithm that needs those yet. For example, merge requires sorted inputs, so the lhs/lhs and rhs/rhs overloads are never needed.

Also, overloads for (lhs_index_type, lhs_index_type) and (rhs_index_type, rhs_index_type) would require this class to own two additional comparators (self comparators for left and right tables).

Hey guys! I stumbled on to this: using self_comparator with strong index types! So we are going to need (lhs_index_type, lhs_index_type) and (rhs_index_type, rhs_index_type) overloads for self_comparator.

These overloads just convert lhs_index_type or lhs_index_type into size_type and call the normal operator()(size_type).

Update: I may not need this as I have another idea to avoid using strong index type, but this is still a relevant need.

cpp/src/search/search.cu

ttnghia · 2022-05-11T04:00:46Z

search.cu currently has merge conflict. I resolved it here: https://github.com/rapidsai/cudf/pull/10656/files#diff-664305c8b9b69efc1edae1d07f4466e6b9af6a3f41760497eb7414ee3ad9b610, and some temporary fix for unit tests here: f3dd8e7

It seems that we must rely on the upcoming weak order comparator from #10793.

…x-type

cpp/include/cudf/table/experimental/row_operators.cuh

ttnghia

Approve, pending CI tests to pass all 😃

jrhemstad

Very clean and concise PR!

bdice · 2022-05-18T12:09:09Z

@gpucibot merge

vyasr · 2022-05-18T18:37:15Z

cpp/include/cudf/table/experimental/row_operators.cuh

+template <typename Index, typename Underlying = std::underlying_type_t<Index>>
+struct strong_index_iterator : public thrust::iterator_facade<strong_index_iterator<Index>,


@bdice I realize that I am late to the party here, but this struct has zero docs. A new reader of the code won't have any idea why this struct exists since the purpose is not conveyed by its implementation, but rather by its existence alone. Could you make a PR with some brief documentation (or maybe just stick that into one of the downstream PRs that either you or @ttnghia is working on)?

Sure thing. I can create a standalone PR for this.

This adds strong index types for equality comparator, along with #10730 to unblock #10548, #10656, and several others nested type feature requests. Authors: - Nghia Truong (https://github.com/ttnghia) - Bradley Dice (https://github.com/bdice) Approvers: - AJ Schmidt (https://github.com/ajschmidt8) - Bradley Dice (https://github.com/bdice) URL: #10883

This extends the `cudf::contains` API to support nested types (lists + structs) with arbitrarily nested levels. As such, `cudf::contains` will work with literally any type of input data. In addition, this fixes null handling of `cudf::contains` with structs column + struct scalar input when the structs column contains null rows at the top level while the scalar key is valid but all nulls at children levels. Closes: #8965 Depends on: * #10730 * #10883 * #10802 * #10997 * NVIDIA/cuCollections#172 * NVIDIA/cuCollections#173 * #11037 * #11356 Authors: - Nghia Truong (https://github.com/ttnghia) - Devavret Makkar (https://github.com/devavret) - Bradley Dice (https://github.com/bdice) - Karthikeyan (https://github.com/karthikeyann) Approvers: - AJ Schmidt (https://github.com/ajschmidt8) - Bradley Dice (https://github.com/bdice) - Yunsong Wang (https://github.com/PointKernel) URL: #10656

This extends the `lists::contains` API to support nested types (lists + structs) with arbitrarily nested levels. As such, `lists::contains` will work with literally any type of input data. In addition, the related implementation has been significantly refactored to facilitate adding new implementation. Closes #8958. Depends on: * #10730 * #10883 * #10999 * #11019 * #11037 Authors: - Nghia Truong (https://github.com/ttnghia) - Bradley Dice (https://github.com/bdice) Approvers: - MithunR (https://github.com/mythrocks) - Bradley Dice (https://github.com/bdice) URL: #10548

bdice added libcudf Affects libcudf (C++/CUDA) code. improvement Improvement / enhancement to an existing function labels Apr 25, 2022

bdice self-assigned this Apr 25, 2022

jrhemstad reviewed Apr 25, 2022

View reviewed changes

cpp/include/cudf/detail/utilities/strong_index.hpp Outdated Show resolved Hide resolved

bdice changed the title ~~Use a strongly typed index for two-table comparators~~ Two-table comparators with strong index types May 2, 2022

bdice force-pushed the strong-index-type branch from 2191031 to 34e4658 Compare May 2, 2022 20:36

Add strong index type.

50b8891

bdice force-pushed the strong-index-type branch from 4652368 to 7a3cbda Compare May 3, 2022 00:20

bdice added 3 commits May 2, 2022 17:23

Revert changes to non-experimental row operators.

b9ed4d7

Use enum for strongly typed index.

d67f17e

Add two table comparator and adapter.

464ed2b

bdice force-pushed the strong-index-type branch from 7a3cbda to 464ed2b Compare May 3, 2022 00:23

This comment was marked as off-topic.

Sign in to view

ttnghia reviewed May 3, 2022

View reviewed changes

cpp/include/cudf/table/experimental/row_operators.cuh Outdated Show resolved Hide resolved

bdice added 4 commits May 2, 2022 19:51

Add friends. :)

b26b318

Apply two-table comparator to search algorithms.

1fd199d

Move shared lhs/rhs logic into launch_search.

18bd9f0

Improve comments, remove old code.

b5b8b39

bdice force-pushed the strong-index-type branch from c461ac9 to b5b8b39 Compare May 3, 2022 04:26

This was referenced May 3, 2022

Add struct utility functions. #10776

Merged

Use column_views instead of column_device_views in binary operations. #10780

Merged

ttnghia reviewed May 4, 2022

View reviewed changes

cpp/include/cudf/table/experimental/row_operators.cuh Outdated Show resolved Hide resolved

rwlee mentioned this pull request May 4, 2022

Return weak orderings from device_row_comparator. #10793

Merged

jrhemstad reviewed May 4, 2022

View reviewed changes

ttnghia reviewed May 10, 2022

View reviewed changes

cpp/src/search/search.cu Outdated Show resolved Hide resolved

ttnghia reviewed May 10, 2022

View reviewed changes

cpp/src/search/search.cu Outdated Show resolved Hide resolved

bdice added 2 commits May 11, 2022 15:32

Merge remote-tracking branch 'upstream/branch-22.06' into strong-inde…

4060b4f

…x-type

Move strong typing code into cudf::experimental::row::lexicographic.

73c4b27

Improve docstrings.

3e81b53

bdice requested review from devavret and ttnghia May 16, 2022 19:35

Use thrust::iterator_facade.

1834095

ttnghia reviewed May 16, 2022

View reviewed changes

cpp/include/cudf/table/experimental/row_operators.cuh Outdated Show resolved Hide resolved

jrhemstad reviewed May 17, 2022

View reviewed changes

cpp/include/cudf/table/experimental/row_operators.cuh Outdated Show resolved Hide resolved

jrhemstad reviewed May 17, 2022

View reviewed changes

cpp/include/cudf/table/experimental/row_operators.cuh Outdated Show resolved Hide resolved

Use const for struct members.

ff26024

ttnghia mentioned this pull request May 17, 2022

Fully support nested types in cudf::contains #10656

Merged

bdice added 4 commits May 17, 2022 15:30

Slim down the strong index layer by using a templated struct.

f779bff

Simplify construction.

157abbc

Use size_type const where possible.

a2ac19d

Require weakly or strongly typed values for lhs_index and rhs_index.

75249e8

bdice requested review from jrhemstad and ttnghia May 17, 2022 20:53

sameerz mentioned this pull request May 17, 2022

[FEA] collect_set on struct[Array] NVIDIA/spark-rapids#5508

Closed

ttnghia approved these changes May 17, 2022

View reviewed changes

Unconstrain template typenames.

bed1162

jrhemstad approved these changes May 18, 2022

View reviewed changes

rapids-bot bot merged commit 7f9d51b into rapidsai:branch-22.06 May 18, 2022

ttnghia mentioned this pull request May 18, 2022

Strong index types for equality comparator #10883

Merged

vyasr reviewed May 18, 2022

View reviewed changes

ttnghia mentioned this pull request May 20, 2022

Support nested types in lists::contains #10548

Merged

devavret mentioned this pull request Jun 6, 2022

[FEA] Story - Supporting row operators on nested types #10186

Closed

GregoryKimball mentioned this pull request Oct 21, 2022

[FEA] Support strong index types in other cudf::experimental::row:: comparators and hashers #11091

Open

GregoryKimball mentioned this pull request May 16, 2023

[FEA] Implement full support for nested types #11844

Closed

		__device__ bool operator()(lhs_index_type const lhs_index,
		rhs_index_type const rhs_index) const noexcept

		template <typename Index, typename Underlying = std::underlying_type_t<Index>>
		struct strong_index_iterator : public thrust::iterator_facade<strong_index_iterator<Index>,

Conversation

bdice commented Apr 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

ttnghia commented May 2, 2022

Uh oh!

This comment was marked as off-topic.

Uh oh!

Uh oh!

jrhemstad May 4, 2022

Choose a reason for hiding this comment

Uh oh!

bdice May 4, 2022

Choose a reason for hiding this comment

Uh oh!

bdice May 11, 2022

Choose a reason for hiding this comment

Uh oh!

ttnghia May 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ttnghia commented May 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ttnghia left a comment

Choose a reason for hiding this comment

Uh oh!

jrhemstad left a comment

Choose a reason for hiding this comment

Uh oh!

bdice commented May 18, 2022

Uh oh!

vyasr May 18, 2022

Choose a reason for hiding this comment

Uh oh!

bdice May 18, 2022

Choose a reason for hiding this comment

Uh oh!

vyasr May 18, 2022

Choose a reason for hiding this comment

Uh oh!

bdice May 18, 2022

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

bdice commented Apr 25, 2022 •

edited

Loading

ttnghia May 19, 2022 •

edited

Loading

ttnghia commented May 11, 2022 •

edited

Loading