Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
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
GH-37378: [C++] Add A Dictionary Compaction Function For DictionaryArray #37418
GH-37378: [C++] Add A Dictionary Compaction Function For DictionaryArray #37418
Changes from 56 commits
1dadf59
cf474f5
aec9ca2
e522841
9853df3
23eed73
d1af639
83a38e1
dafd8c8
cf29aae
3fa7abc
74e4fb1
ef1f335
354b5b0
38a7bcc
93f4029
1fe45d0
46959c2
a3dbf0f
c6fd4f2
73cc864
092a38c
dc6244b
529246d
a826666
83dc6fd
8d2129a
f39c118
d9f3ebf
4ef8457
8901bd3
456c217
69ca6a0
b59fe45
ffda004
0ed463a
ea88395
0e84226
126761e
a8c9642
f6da942
065cee4
ce9876b
1384fe9
ea3c416
e236e96
4be9f7b
68137d4
731b583
443ce45
90f27f1
c8a0480
c48e57e
16854e2
d74f17d
a7d5c60
168a891
0cffac0
c1a4635
1665144
4562d0f
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Checking here enables skipping the rest of the dictionary, which is good. However I think it'd also be useful to detect usage of only a slice of the dictionary. If you'd prefer not to handle that in this PR, please write a follow up issue
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, @bkietz, do you mean if we find it just use only a slice of dictionay, we use
slice()
instead ofTake
? I prefer to leave it as an new issue. Since we have another PR which is wating for this PR to be merged.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we just use
Slice
outside theCompact
to handling this?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I write a follow up issue #38247 to handle the optimization
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add a test for invalid input type?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. We can't create a DictionaryArray with an invalid index type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I see a
Here, but seems it cannot be called