dictionary encoded group by's#20563
Draft
alexanderbianchi wants to merge 1 commit intoapache:mainfrom
Draft
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Which issue does this PR close?
Partially addresses #7647
Rationale for this change
Dictionary-encoded GROUP BY columns currently fall back to
GroupValuesRows(Arrow'sRowConverter), which:cast()on emitGroupValuesColumnfast pathThis is the same problem that PR #8291 attempted to solve by materializing dictionaries at the
AggregateExeclevel. That approach was reverted because it changed the output schema (Dictionary(Int32, Utf8)→Utf8), which broke UNION queries with aRowConverter column schema mismatcherror (#8738).See also the related arrow-rs discussion on stateless row conversion (arrow-rs #4811, arrow-rs #4813, arrow-rs #4819).
What changes are included in this PR?
This PR takes a different approach: instead of changing schemas or materializing dictionaries at the plan level, it adds dictionary support internally to
GroupValuesColumnvia a thin wrapper that resolves dictionary keys on-demand.How it differs from PR #8291
AggregateExeclevel — changes output schemaGroupValuesColumn— completely internalDictionaryto value typeDictionaryGroupValuesColumnImplementation
DictionaryGroupValueBuilder<K>— a newGroupColumnwrapper (~180 lines) that:DictionaryArray<K>inputskeys.value(row).as_usize())ByteGroupValueBuilderfor Utf8) for storage and comparisonThe wrapper has fast paths for the common case (no null dictionary keys) where it remaps indices in bulk and delegates to the inner builder's vectorized operations.
make_group_column()— replaces the oldinstantiate_primitive!macro with a function that returnsResult<Box<dyn GroupColumn>>. This naturally supports the recursive Dictionary wrapping (create inner builder for value type, wrap withDictionaryGroupValueBuilder) and simplifiesintern()to a 3-line loop.supported_type()— now acceptsDictionary(_, vt)where the value type is itself supported.No changes to the emit path — it already re-encodes value-type arrays back to Dictionary via
cast().Files changed
multi_group_by/dictionary.rsDictionaryGroupValueBuilder<K>wrappermulti_group_by/mod.rssupported_type(),make_group_column(), simplifiedintern(), 3 new testsgroup_values/mod.rsmod row→pub mod row(benchmark visibility)benches/dict_group_values.rsCargo.tomlAre these changes tested?
Yes:
test_supported_type_dictionary— verifiessupported_typeaccepts/rejects dictionary types correctlytest_intern_with_dictionary_columns— multi-column grouping withDictionary(Int32, Utf8)+Int64, overlapping groups across batches, verifies group indices and emit output typestest_dictionary_null_handling— null dictionary keys produce null groups, verified across batchesgroup_valuesunit tests continue to passaggregates::dict_nulls) passAre there any user-facing changes?
No. The output schema, output types, and query results are identical. Dictionary-encoded GROUP BY columns now route through
GroupValuesColumninstead ofGroupValuesRows, which is purely an internal performance optimization.Benchmark results
Benchmark compares three paths for GROUP BY on
Dictionary(Int32, Utf8)columns:column_utf8:GroupValuesColumnwith plain Utf8 (fast-path baseline)column_dict:GroupValuesColumnwith Dictionary — this PRrows_dict:GroupValuesRowswith Dictionary — current mainParameters: 10 batches per iteration, cardinalities 50 / 1,000 / 10,000.
At low cardinality (50 distinct values),
rows_dictandcolumn_dictare comparable — theRowConverteris efficient when it has few values to intern. At moderate-to-high cardinality (1,000–10,000),column_dictis 1.1–1.6x faster than the currentGroupValuesRowspath, with the advantage growing as cardinality increases.column_dictis also consistently faster thancolumn_utf8(plain strings) because the dictionary's compact values array is more cache-friendly than scanning the full batch.Run with:
cargo bench -p datafusion-physical-plan --bench dict_group_values --features test_utils