add `garbage_collect_dictionary` to `arrow-select` #7716

davidhewitt · 2025-06-19T13:34:40Z

Which issue does this PR close?

Closes #7683

What changes are included in this PR?

I add arrow_select::dictionary::{garbage_collect_dictionary, garbage_collect_any_dictionary}.

The latter is not strictly necessary but I expect it will be helpful to users.

Are there any user-facing changes?

New APIs, documented.

davidhewitt · 2025-06-19T13:36:15Z

arrow-select/src/dictionary.rs

+    // Create a new values array with the masked values
+    let values = filter(dictionary.values(), &BooleanArray::new(mask, None))?;


This use of filter is how I avoid casting to a concrete value type.

There is also an Interner which is used in other kernels for dictionary deduplication, but I chose not to use it because:

it doesn't support all data types, and

this implementation is more consistent with GenericByteViewArray::gc

arrow-select/src/dictionary.rs

davidhewitt · 2025-06-19T15:13:48Z

Thanks, those suggestions have led to a much better implementation.

davidhewitt · 2025-06-20T17:22:55Z

Looks like legitimate MSRV bug, will fix monday.

davidhewitt · 2025-06-23T08:24:32Z

arrow-select/src/dictionary.rs

+    // FIXME: this is a workaround for MSRV Rust versions below 1.86 where trait upcasting is not stable.
+    // From 1.86 onward, `&dyn AnyDictionaryArray` can be directly passed to `downcast_dictionary_array!`.
+    let dictionary = &*dictionary.slice(0, dictionary.len());


This fixed MSRV without materially changing the PR, I'm open to alternatives like just removing garbage_collect_any_dictionary for now.

It seems fine to me

davidhewitt · 2025-07-10T09:30:25Z

ping @alamb @tustvold just wanted to make sure this doesn't get lost in the backlog please 🙏

davidhewitt · 2025-07-10T13:28:43Z

Merged main which should hopefully fix the clippy failures.

alamb

Thank you @davidhewitt -- I think this looks great.

I had some comment suggestions and questions about pub(crate) but I don't think they are required to merge.

Can you please merge up from main to get a clean CI run on this PR?

Thanks for your patience

arrow-select/src/dictionary.rs

alamb · 2025-07-10T13:43:16Z

arrow-select/src/dictionary.rs

+    // FIXME: this is a workaround for MSRV Rust versions below 1.86 where trait upcasting is not stable.
+    // From 1.86 onward, `&dyn AnyDictionaryArray` can be directly passed to `downcast_dictionary_array!`.
+    let dictionary = &*dictionary.slice(0, dictionary.len());


It seems fine to me

alamb · 2025-07-10T13:44:18Z

arrow-select/src/dictionary.rs

 ///
 /// `len` is the total length of the merged output
-pub fn should_merge_dictionary_values<K: ArrowDictionaryKeyType>(
+pub(crate) fn should_merge_dictionary_values<K: ArrowDictionaryKeyType>(


Iis this change still needed? I didn't see should_merge_dictionary_values used anyhwere in this PR

Yes - I made dictionary.rs a pub mod so that arrow_select::dictionary::garbage_collect_dictionary can be the public API path.

alamb · 2025-07-10T13:45:10Z

arrow-select/src/dictionary.rs

 }

-pub struct MergedDictionaries<K: ArrowDictionaryKeyType> {
+pub(crate) struct MergedDictionaries<K: ArrowDictionaryKeyType> {


Is this change needed? I didn't see MergedDictionaries used anywhere else

Same as #7716 (comment)

Co-authored-by: Andrew Lamb <[email protected]>

davidhewitt · 2025-07-10T18:40:58Z

Thansk, I think should be good to go now 👍

alamb · 2025-07-10T21:12:12Z

Looks like there is a small cargo fmt issue to fix

davidhewitt · 2025-07-10T21:34:07Z

Done 👍

davidhewitt · 2025-07-11T05:43:11Z

Wow CI has not been kind here! Merged main again.

davidhewitt · 2025-07-11T10:26:52Z

(Tested locally with latest nightly, looks like those docs should build ok 🤔)

davidhewitt · 2025-07-11T10:54:56Z

Ungh that was user error on my part - ran cargo doc instead of cargo +nightly doc 🤦 . I repro this locally, will either send a PR myself to fixup later or maybe someone else sends one first.

davidhewitt · 2025-07-11T10:55:14Z

(and that was a fat finger error 🙃 - what a PR!)

alamb · 2025-07-11T19:17:37Z

I think main had a CI failure on docs that @viirya fixed in

Fix current CI failure #7898

Hopefully this one gets a clean(enough) run and I'll merge it in

alamb · 2025-07-11T19:28:05Z

The docs CI failure https://github.com/apache/arrow-rs/actions/runs/16218327533/job/45824214545?pr=7716 is what was fixed. It think we can merge this PR in safely and it will not fail on main

alamb · 2025-07-11T19:39:12Z

🚀

alamb · 2025-07-11T19:39:26Z

Thanks for you patience @davidhewitt

davidhewitt · 2025-07-11T21:08:14Z

All good, likewise and thanks for the merge!

adriangb · 2025-10-27T15:07:30Z

arrow-select/src/dictionary.rs

+pub fn garbage_collect_any_dictionary(
+    dictionary: &dyn AnyDictionaryArray,
+) -> Result<ArrayRef, ArrowError> {


Any particular reason we didn't ad this as a method on AnyDictionaryArray itself or closer to it? I guess that requires garbage_collect_dictionary to also move, but I'm also not sure why that one is in arrow-select. It would be nice if these APIs were the same as the view type APIs (array.gc()).

The implementation uses arrow_select::filter kernels, it would be undesirable to move those.

add garbage_collect_dictionary to arrow-select

6ee4216

github-actions bot added the arrow Changes to the arrow crate label Jun 19, 2025

davidhewitt commented Jun 19, 2025

View reviewed changes

tustvold reviewed Jun 19, 2025

View reviewed changes

arrow-select/src/dictionary.rs Show resolved Hide resolved

tustvold reviewed Jun 19, 2025

View reviewed changes

arrow-select/src/dictionary.rs Outdated Show resolved Hide resolved

[tustvold] review feedback

3d05a32

workaround for MSRV

ad8c45e

davidhewitt commented Jun 23, 2025

View reviewed changes

davidhewitt requested a review from tustvold June 30, 2025 09:26

Merge branch 'main' into dh/dictionary-gc

b728ec8

alamb approved these changes Jul 10, 2025

View reviewed changes

Update arrow-select/src/dictionary.rs

3f324c7

Co-authored-by: Andrew Lamb <[email protected]>

fmt

10ba2f5

Merge branch 'apache:main' into dh/dictionary-gc

f60344a

davidhewitt closed this Jul 11, 2025

davidhewitt reopened this Jul 11, 2025

alamb merged commit 6d11232 into apache:main Jul 11, 2025
49 of 51 checks passed

alamb mentioned this pull request Jul 28, 2025

Add DictionaryArray::gc method #7683

Closed

adriangb reviewed Oct 27, 2025

View reviewed changes

		// Create a new values array with the masked values
		let values = filter(dictionary.values(), &BooleanArray::new(mask, None))?;

add garbage_collect_dictionary to arrow-select #7716

add garbage_collect_dictionary to arrow-select #7716

Uh oh!

Conversation

davidhewitt commented Jun 19, 2025

Which issue does this PR close?

What changes are included in this PR?

Are there any user-facing changes?

Uh oh!

davidhewitt Jun 19, 2025 • 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!

davidhewitt commented Jun 19, 2025

Uh oh!

davidhewitt commented Jun 20, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidhewitt commented Jul 10, 2025

Uh oh!

davidhewitt commented Jul 10, 2025

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidhewitt commented Jul 10, 2025

Uh oh!

alamb commented Jul 10, 2025

Uh oh!

davidhewitt commented Jul 10, 2025

Uh oh!

davidhewitt commented Jul 11, 2025

Uh oh!

davidhewitt commented Jul 11, 2025

Uh oh!

davidhewitt commented Jul 11, 2025

Uh oh!

davidhewitt commented Jul 11, 2025

Uh oh!

alamb commented Jul 11, 2025

Uh oh!

alamb commented Jul 11, 2025

Uh oh!

alamb commented Jul 11, 2025

Uh oh!

Uh oh!

alamb commented Jul 11, 2025

Uh oh!

davidhewitt commented Jul 11, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

add `garbage_collect_dictionary` to `arrow-select` #7716

add `garbage_collect_dictionary` to `arrow-select` #7716

davidhewitt Jun 19, 2025 •

edited

Loading