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.

Sign up for GitHub

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

Jump to bottom

[FEA] Support struct and list columns in element_equality_comparator #8683

Closed
rwlee opened this issue Jul 7, 2021 · 3 comments · Fixed by #8962
Closed

[FEA] Support struct and list columns in element_equality_comparator #8683

rwlee opened this issue Jul 7, 2021 · 3 comments · Fixed by #8962
Assignees
@rwlee
Labels
feature request New feature or request libcudf Affects libcudf (C++/CUDA) code.

Comments

@rwlee
Copy link
Contributor

rwlee commented Jul 7, 2021

From cpp/include/cudf/table/row_operators.cuh, add functionality to element_equality_comparator and row_equality_comparator to compare struct and list elements. Currently they are quietly caught as non-is_equality_comparable types and return false representing unequal elements.

row_equality_comparator is used in #8652 for rank and dense rank scan aggregations. Both would benefit from this change.
@rwlee rwlee added feature request New feature or request Needs Triage Need team to review and classify libcudf Affects libcudf (C++/CUDA) code. labels Jul 7, 2021
@jrhemstad
Copy link
Contributor

We have explored this at length and unfortunately this is not possible as it would require arbitrary recursion.

@rwlee
Copy link
Contributor Author

rwlee commented Jul 8, 2021

Is there a reason to not support the simple cases of lists and structs? e.g. structs without lists or other structs as elements and lists that are not lists of lists or lists of structs.

@jrhemstad
Copy link
Contributor

jrhemstad commented Jul 8, 2021 •
edited
Loading

structs without lists or other structs as elements and lists that are not lists of lists or lists of structs.

Yes, this case we can support by special casing the dispatch for struct and lists. Arbitrary nesting is where you get into trouble.

@beckernick beckernick removed the Needs Triage Need team to review and classify label Jul 12, 2021
@rwlee rwlee self-assigned this Jul 28, 2021
@rwlee rwlee mentioned this issue Aug 5, 2021
Merged
@rwlee rwlee linked a pull request Aug 5, 2021 that will close this issue
Support nested structs in rank and dense rank #8962
Merged
rapids-bot bot pushed a commit that referenced this issue Sep 14, 2021
@rwlee
Support nested structs in rank and dense rank (#8962)
eae76cf 
Follow on to #8652 for nested struct support using, partially removing the need for #8683.

This change simplifies the rank algorithm by assuming `superimpose_parent_nulls` has been ran on the struct column. This removes the need for separate logic that ensures we are not comparing elements covered by a parent column's null mask.

Authors:
  - https://github.com/rwlee

Approvers:
  - Vyas Ramasubramani (https://github.com/vyasr)
  - https://github.com/nvdbaranec
  - Mike Wilson (https://github.com/hyperbolic2346)
  - Mark Harris (https://github.com/harrism)

URL: #8962
@devavret devavret mentioned this issue Feb 1, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@rwlee rwlee

Labels
feature request New feature or request libcudf Affects libcudf (C++/CUDA) code.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Support nested structs in rank and dense rank rwlee/cudf
3 participants
@beckernick @rwlee @jrhemstad