Add shallow hash function and shallow equality comparison for column_view #9185

karthikeyann · 2021-09-07T18:51:06Z

Fixes #9140
Added shallow_hash(column_view)
Added unit tests

It computes hash values based on the shallow states of column_view:
type, size, data pointer, null_mask pointer, offset, and the hash value of the children.
null_count is not used since it is a cached value and it may vary based on contents of null_mask, and may be pre-computed or not.

Fixes #9139
Added is_shallow_equivalent(column_view, column_view) ~~shallow_equal~~
Added unit tests

It compares two column_views based on the shallow states of column_view:
type, size, data pointer, null_mask pointer, offset, and the column_view of the children.
null_count is not used since it is a cached value and it may vary based on contents of null_mask, and may be pre-computed or not.

GPUtester · 2021-09-07T18:51:09Z

Can one of the admins verify this patch?

jrhemstad · 2021-09-07T20:12:15Z

cpp/src/column/column_view.cpp

+  combine_hash(hash, std::hash<void const*>{}(input.head()));
+  combine_hash(hash, std::hash<void const*>{}(input.null_mask()));


I don't think you should include the head or the null_mask pointer in the hash when the size is 0. Those pointers could be garbage if the column doesn't have any elements. In fact, you should explicitly add a test for this.

then slice(col_1, {0,0})[0] == slice(col_2, {0,0})[0]. Is this correct behavior?

I'm not sure I understand your example. Are you saying two columns without any elements will produce the same hash value? I think that's okay. With an empty column, there is no physical column that we're viewing, only a conceptual one. So all empty column_views of the same type conceptually view the same column.

Yes. That's what I meant.
shallow_hash(slice(column_view(col_1), {0,0})[0]) == shallow_hash(slice(column_view(col_1_copy), {0,0})[0]) fails for nested types only because children sizes may not be zero after slicing even if parent size is zero, and hence child.data is compared and they are different.

auto col_new = std::make_unique<cudf::column>(*col); auto col_new_view = col_new->view(); auto col_sliced = cudf::slice(col_view, {0, 0, 1, 1, col_view.size(), col_view.size()}); auto col_new_sliced = cudf::slice(col_new_view, {0, 0, 1, 1, col_view.size(), col_view.size()}); EXPECT_EQ(shallow_hash(col_sliced[0]), shallow_hash(col_new_sliced[0]));

parent.size() could be propagated to shallow_hash(children) too, but it does not seem right. (propagate one level or all the way to leaf).
We can't simply ignore children altogether because 2 struct column view with different children types should be assumed to have different hash, even both are empty.

I'm not suggesting you ignore children, but instead, ignore the values of data() and null_mask() when the column's size is zero. If two columns c0, c1 both have c0.size() == 0 == c1.size() but they have children with different sizes/types/offsets/etc. then they are not shallow equal. This should be taken care of automatically when you recurse to the children and see that some of the shallow state is different.

commenting outcome of offline discussion:

for empty column, ignore the children, check types are same, ignore size, offset, pointers.
only look at the nested data types.

call it is_shallow_equivalent instead of is_shallow_equal.

cpp/include/cudf/column/column_view.hpp

cpp/src/column/column_view.cpp

cpp/tests/column/column_view_shallow_test.cpp

Co-authored-by: Jake Hemstad <[email protected]>

…/cudf into fea-shallow_equal_columnview

cpp/src/column/column_view.cpp

Co-authored-by: Jake Hemstad <[email protected]>

…/cudf into fea-shallow_hash_columnview

codecov · 2021-09-18T11:14:29Z

Codecov Report

Merging #9185 (e5bfd50) into branch-21.10 (3ee3ecf) will decrease coverage by 0.02%.
The diff coverage is 0.00%.

❗ Current head e5bfd50 differs from pull request most recent head 1a5f367. Consider uploading reports for the commit 1a5f367 to get more accurate results

@@               Coverage Diff                @@
##           branch-21.10    #9185      +/-   ##
================================================
- Coverage         10.85%   10.83%   -0.03%     
================================================
  Files               115      116       +1     
  Lines             19158    18781     -377     
================================================
- Hits               2080     2035      -45     
+ Misses            17078    16746     -332

Impacted Files	Coverage Δ
python/cudf/cudf/__init__.py	`0.00% <ø> (ø)`
python/cudf/cudf/_lib/__init__.py	`0.00% <ø> (ø)`
python/cudf/cudf/core/multiindex.py	`0.00% <0.00%> (ø)`
python/cudf/cudf/io/__init__.py	`0.00% <0.00%> (ø)`
python/cudf/cudf/io/text.py	`0.00% <0.00%> (ø)`
python/cudf/cudf/utils/ioutils.py	`0.00% <0.00%> (ø)`
python/custreamz/custreamz/tests/conftest.py	`71.42% <0.00%> (-7.15%)`	⬇️
python/custreamz/custreamz/tests/test_kafka.py	`35.71% <0.00%> (-7.15%)`	⬇️
...ython/custreamz/custreamz/tests/test_dataframes.py	`96.97% <0.00%> (-2.42%)`	⬇️
python/dask_cudf/dask_cudf/backends.py	`82.31% <0.00%> (-0.43%)`	⬇️
... and 53 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f08d6f1...1a5f367. Read the comment docs.

karthikeyann · 2021-09-18T18:10:03Z

added license for SWIPAT.

cpp/src/column/column_view.cpp

vyasr

I agree with David's last suggestion, and I had one minor comment as well, but aside from that this looks good to me now.

cpp/src/column/column_view.cpp

davidwendt · 2021-09-21T15:00:31Z

cpp/src/column/column_view.cpp

+                    rhs.child_end(),
+                    [is_empty](auto const& lhs_child, auto const& rhs_child) {
+                      return shallow_equivalent_impl(lhs_child, rhs_child, is_empty);
+                    });


Maybe this has already been discussed but why not implement this function like:

bool shallow_equivalent_impl(column_view const& lhs, column_view const& rhs, bool is_parent_empty = false) { return shallow_hash_impl(lhs) == shallow_hash_impl(rhs); } Should not the `hash` values and `equivalent` results be consistent anyway? The `hash_combine` function does not look like it would be a significant impact here.

It seems this function is also accessing child size/offset values even if the parent is empty. And keeping the hash and equivalent functions in sync may be challenging.

equivalent cannot use hash because of hash collison. 2 column_view that are not equivalent may end up having same hash. 2 equivalent column_view will have same hash, but vice versa may not be true always.

cpp/src/column/column_view.cpp

Co-authored-by: David Wendt <[email protected]>

jrhemstad · 2021-09-22T19:56:56Z

@gpucibot merge

… column_view (#9185)" This reverts commit 1cb527f.

… column_view (#9185)" (#9283) Reverts #9185 More details on PR #9185 Authors: - Karthikeyan (https://github.com/karthikeyann) Approvers: - Jake Hemstad (https://github.com/jrhemstad) - Devavret Makkar (https://github.com/devavret) - Conor Hoekstra (https://github.com/codereport) URL: #9283

…ed on column_view instead of requests. (#9195) This PR updates groupby result_cache to use `pair<column_view, aggregation>` as key to unordered_map. This allows to cache intermediate results based on the column view. So, it is possible to cache children column_view results and can be resused in other aggregation_request. Depends on #9185 shallow_hash and is_shallow_equivalent are used for column_view. Additional context: This change is required to cache children column intermediate results in #9154 and allows to be shared across multiple aggregation requests. Authors: - Karthikeyan (https://github.com/karthikeyann) Approvers: - Jake Hemstad (https://github.com/jrhemstad) - Vyas Ramasubramani (https://github.com/vyasr) URL: #9195

karthikeyann added 3 commits September 8, 2021 00:03

add shallow_hash(column_view)

d421d6d

add CompoundTypes to type_lists

9c4a9f3

add shallow_hash tests

a3dd235

karthikeyann requested a review from a team as a code owner September 7, 2021 18:51

karthikeyann requested review from harrism and vyasr September 7, 2021 18:51

github-actions bot added CMake CMake build issue libcudf Affects libcudf (C++/CUDA) code. labels Sep 7, 2021

karthikeyann added 3 - Ready for Review Ready for review by team 4 - Needs Review Waiting for reviewer to review or respond feature request New feature or request non-breaking Non-breaking change and removed libcudf Affects libcudf (C++/CUDA) code. CMake CMake build issue labels Sep 7, 2021

add column copy test

2365d07

github-actions bot added CMake CMake build issue libcudf Affects libcudf (C++/CUDA) code. labels Sep 7, 2021

add shallow_equal(column_view) and tests

88726a4

karthikeyann mentioned this pull request Sep 7, 2021

Add shallow equality comparison for column_view #9187

Closed

jrhemstad reviewed Sep 7, 2021

View reviewed changes

cpp/include/cudf/column/column_view.hpp Outdated Show resolved Hide resolved

jrhemstad reviewed Sep 7, 2021

View reviewed changes

cpp/include/cudf/column/column_view.hpp Outdated Show resolved Hide resolved

jrhemstad reviewed Sep 7, 2021

View reviewed changes

cpp/src/column/column_view.cpp Outdated Show resolved Hide resolved

vyasr reviewed Sep 7, 2021

View reviewed changes

cpp/src/column/column_view.cpp Outdated Show resolved Hide resolved

cpp/tests/column/column_view_shallow_test.cpp Outdated Show resolved Hide resolved

cpp/tests/column/column_view_shallow_test.cpp Outdated Show resolved Hide resolved

karthikeyann and others added 3 commits September 8, 2021 12:42

Update cpp/include/cudf/column/column_view.hpp

d9a8bd7

Co-authored-by: Jake Hemstad <[email protected]>

ignore data, nullmask, offset if parent size is empty

7e7f250

Merge branch 'fea-shallow_hash_columnview' of github.com:karthikeyann…

d1d5c3c

…/cudf into fea-shallow_equal_columnview

jrhemstad reviewed Sep 13, 2021

View reviewed changes

cpp/src/column/column_view.cpp Outdated Show resolved Hide resolved

karthikeyann and others added 3 commits September 18, 2021 13:53

Apply suggestions from code review (jake)

1fbe3fc

Co-authored-by: Jake Hemstad <[email protected]>

include cleanup

fc3cc6b

Merge branch 'fea-shallow_hash_columnview' of github.com:karthikeyann…

eb2b0db

…/cudf into fea-shallow_hash_columnview

karthikeyann requested review from vyasr, jrhemstad and davidwendt September 20, 2021 04:36

davidwendt reviewed Sep 20, 2021

View reviewed changes

cpp/src/column/column_view.cpp Outdated Show resolved Hide resolved

vyasr approved these changes Sep 20, 2021

View reviewed changes

cpp/src/column/column_view.cpp Outdated Show resolved Hide resolved

address review comments

9581525

karthikeyann requested a review from davidwendt September 20, 2021 23:13

jrhemstad approved these changes Sep 20, 2021

View reviewed changes

davidwendt reviewed Sep 21, 2021

View reviewed changes

cpp/src/column/column_view.cpp Outdated Show resolved Hide resolved

davidwendt reviewed Sep 21, 2021

View reviewed changes

cpp/src/column/column_view.cpp Show resolved Hide resolved

Update cpp/src/column/column_view.cpp

1a5f367

Co-authored-by: David Wendt <[email protected]>

karthikeyann removed 3 - Ready for Review Ready for review by team 4 - Needs Review Waiting for reviewer to review or respond labels Sep 21, 2021

karthikeyann requested a review from davidwendt September 21, 2021 16:03

davidwendt approved these changes Sep 22, 2021

View reviewed changes

rapids-bot bot merged commit 1cb527f into rapidsai:branch-21.10 Sep 22, 2021

karthikeyann added a commit that referenced this pull request Sep 23, 2021

Revert "Add shallow hash function and shallow equality comparison for…

10213dc

… column_view (#9185)" This reverts commit 1cb527f.

karthikeyann mentioned this pull request Sep 23, 2021

Revert "Add shallow hash function and shallow equality comparison for column_view (#9185)" #9283

Merged

vyasr mentioned this pull request Sep 27, 2021

Add shallow hash function and shallow equality comparison for column_view #9312

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add shallow hash function and shallow equality comparison for column_view #9185

Add shallow hash function and shallow equality comparison for column_view #9185

karthikeyann commented Sep 7, 2021 •

edited

Loading

GPUtester commented Sep 7, 2021

jrhemstad Sep 7, 2021

karthikeyann Sep 8, 2021

jrhemstad Sep 8, 2021

karthikeyann Sep 8, 2021 •

edited

Loading

jrhemstad Sep 8, 2021 •

edited

Loading

karthikeyann Sep 17, 2021

codecov bot commented Sep 18, 2021 •

edited

Loading

karthikeyann commented Sep 18, 2021

vyasr left a comment

davidwendt Sep 21, 2021

karthikeyann Sep 21, 2021

jrhemstad commented Sep 22, 2021

		combine_hash(hash, std::hash<void const*>{}(input.head()));
		combine_hash(hash, std::hash<void const*>{}(input.null_mask()));

Add shallow hash function and shallow equality comparison for column_view #9185

Add shallow hash function and shallow equality comparison for column_view #9185

Conversation

karthikeyann commented Sep 7, 2021 • edited Loading

GPUtester commented Sep 7, 2021

jrhemstad Sep 7, 2021

Choose a reason for hiding this comment

karthikeyann Sep 8, 2021

Choose a reason for hiding this comment

jrhemstad Sep 8, 2021

Choose a reason for hiding this comment

karthikeyann Sep 8, 2021 • edited Loading

Choose a reason for hiding this comment

jrhemstad Sep 8, 2021 • edited Loading

Choose a reason for hiding this comment

karthikeyann Sep 17, 2021

Choose a reason for hiding this comment

codecov bot commented Sep 18, 2021 • edited Loading

Codecov Report

karthikeyann commented Sep 18, 2021

vyasr left a comment

Choose a reason for hiding this comment

davidwendt Sep 21, 2021

Choose a reason for hiding this comment

karthikeyann Sep 21, 2021

Choose a reason for hiding this comment

jrhemstad commented Sep 22, 2021

karthikeyann commented Sep 7, 2021 •

edited

Loading

karthikeyann Sep 8, 2021 •

edited

Loading

jrhemstad Sep 8, 2021 •

edited

Loading

codecov bot commented Sep 18, 2021 •

edited

Loading