Skip to content

ARROW-10403: [C++] Implement unique kernel for non-uniform chunked dictionary arrays - #9683

Closed
rok wants to merge 2 commits into
apache:masterfrom
rok:ARROW-10403
Closed

ARROW-10403: [C++] Implement unique kernel for non-uniform chunked dictionary arrays#9683
rok wants to merge 2 commits into
apache:masterfrom
rok:ARROW-10403

Conversation

@rok

@rokrok commented Mar 12, 2021

Copy link
Copy Markdown
Member

@github-actions

Copy link
Copy Markdown

@rok

rok commented Mar 12, 2021

Copy link
Copy Markdown
MemberAuthor

@nealrichardson what do you think about this approach? It introduces overhead to because it transposes dictionary indices but it gives us value_counts.

@nealrichardson

Copy link
Copy Markdown
Member

I'm not familiar with this C++ code so I'll let others comment (cc @pitrou@bkietz@michalursa). It looks like the issue is only with ChunkedArrays where the chunks have different dictionaries? My instinct is that, rather than unifying first and then determining unique values/counting/hashing, what if we could do the aggregation on each chunk first and then unify the results? That would be a smaller amount of data to manipulate.

@rokrok closed this Mar 12, 2021
@rokrok reopened this Mar 12, 2021
@rok

rok commented Mar 12, 2021

Copy link
Copy Markdown
MemberAuthor

My instinct is that, rather than unifying first and then determining unique values/counting/hashing, what if we could do the aggregation on each chunk first and then unify the results? That would be a smaller amount of data to manipulate.

Indeed unifying over all chunks first and then transposing individual chunk indices would be a better idea!

I'm still a bit unfamiliar with kernel mechanics but I'm thinking implementing a new kernel for chunked DictionaryArrays with different dictionaries will be the best way to go for this.

@pitrou

Copy link
Copy Markdown
Member

There are indeed two possible approaches:

  • unify all chunks first, and then run the unique kernel over the transposed incides (as proposed by @rok)
  • run the unique kernel over the original chunks, and then hash-aggregate the unique results of the different chunks (in effect SELECT sum(counts) GROUP BY values)

The second approach could be faster in the (unusual?) cases where only a small subset of dictionary values actually appear in the data. If most dictionary values are used, both cases should have similar performance, though.

Since we don't have a generic hash-aggregate yet, the first approach sounds good enough.
(also note that unique is in itself a special case of hash-aggregation)

cc @bkietz for opinions

@rok

rok commented Mar 22, 2021

Copy link
Copy Markdown
MemberAuthor

Since we don't have a generic hash-aggregate yet, the first approach sounds good enough.
(also note that unique is in itself a special case of hash-aggregation)

Shall I then fix CI issues and we proceed with the first approach?
Or do we rather put effort into generic hash-aggregate?

@pitrou

Copy link
Copy Markdown
Member

You can fix CI issus IMHO.

@rok
rok marked this pull request as ready for review March 25, 2021 16:19
@rok

rok commented Mar 25, 2021

Copy link
Copy Markdown
MemberAuthor

This is ready for review - the java issue appears to be a flaky upload.

@rok

rok commented Mar 29, 2021

Copy link
Copy Markdown
MemberAuthor

ping :)

@rok
rokforce-pushed the ARROW-10403 branch 3 times, most recently from 293a6fc to 4d00295CompareApril 7, 2021 22:29
@pitroupitrou changed the title ARROW-10403: [C++] Implement unique kernel for dictionary typeARROW-10403: [C++] Implement unique kernel for non-uniform chunked dictionary arraysApr 8, 2021

@pitroupitrou left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

+1. Sorry for the delay, will merge if CI is green.

@pitroupitrou closed this in b24cff9Apr 8, 2021
@rok

rok commented Apr 8, 2021

Copy link
Copy Markdown
MemberAuthor

Thanks @pitrou!
Should we make a jira for the generic hash aggregate?

@pitrou

Copy link
Copy Markdown
Member

Yes, please do!

@rok

rok commented Apr 8, 2021

Copy link
Copy Markdown
MemberAuthor

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@rok@nealrichardson@pitrou