Re: [I] Materialize Dictionaries in Group Keys [datafusion]

["Rich-T-kid (via GitHub)" <gi...@apache.org>]

Rich-T-kid commented on issue #7647:
URL: https://github.com/apache/datafusion/issues/7647#issuecomment-5319896143

   > Related to this issue: on datafusion 54 a dictionary group key whose combined cardinality across partitions exceeds its key width fails hard, rather than just being suboptimal.
   > 
   > ## Repro (pure datafusion + pyarrow, no network)
   > import numpy as np, pyarrow as pa
   > from datafusion import SessionContext
   > 
   > PER_PARTITION, N_PARTITIONS = 100, 50  # combined 5000 >> Int8 max (127)
   > parts = []
   > for p in range(N_PARTITIONS):
   >     values = pa.array(np.arange(p*PER_PARTITION, p*PER_PARTITION+PER_PARTITION, dtype=np.int64))
   >     keys = pa.array(np.arange(PER_PARTITION, dtype=np.int8))
   >     k = pa.DictionaryArray.from_arrays(keys, values)  # valid Dictionary(Int8, Int64) per batch
   >     parts.append(pa.record_batch({"k": k, "v": pa.array(np.ones(PER_PARTITION))}))
   > 
   > ctx = SessionContext()
   > ctx.register_record_batches("t", [[b] for b in parts])
   > ctx.sql("SELECT k, SUM(v) FROM t GROUP BY k").to_arrow_table()
   > # -> Arrow error: Dictionary key bigger than the key type
   > Each partition is a valid `Dictionary(Int8, Int64)` (<=128 distinct), but the partitions hold disjoint values, so the combined group-key set is 5000.
   > 
   > ## Mechanism (datafusion 54)
   > * `Aggregate::group_fields` (physical-plan/src/aggregates/mod.rs) keeps the dictionary type for the group key.
   > * `GroupValuesRows::emit` decodes the keys via the RowConverter, then `dictionary_encode_if_necessary` (group_values/row.rs) re-encodes them back into the narrow dictionary because the group schema says so.
   > * Across `RepartitionExec: Hash` / `FinalPartitioned`, the combined set exceeds Int8, so the re-encode overflows. arrow-rs is behaving correctly here (`concat` is type-preserving, so it errors in `merge_dictionary_values` rather than silently widening); the decision to keep a narrow key for the combined group keys is the datafusion side.
   > 
   > `#8291` materialized dict group keys to the value type and avoided this; that path is not present in 54. This issue and `#21765` are moving toward keeping dictionaries and optimizing on them, which is great for low cardinality but needs to handle the case where the combined cardinality outgrows the key width.
   > 
   > ## Question before I send a PR
   > Two ways to fix it, and I would rather match your direction:
   > 
   > * **A. Decode** dict group keys to their value type at plan time (restores `#8291` semantics). Simple, output type changes to the value type.
   > * **B. Widen** the group key to a safe key width (e.g. Int32/Int64) and keep a dictionary output (aligns with `#21765`). Preserves "group by dict returns dict".
   > 
   > I have both a candidate patch and a sqllogictest ready. Which direction do you prefer for the aggregate group-key path? Happy to open the PR against whichever you pick.
   
   you may be interested in this issue https://github.com/apache/datafusion/issues/23127


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org