lyang24 opened a new pull request, #10862: URL: https://github.com/apache/arrow-rs/pull/10862
# Which issue does this PR close? N/A — small, self-contained perf fix. Happy to open one if you'd prefer it tracked. # Rationale for this change When the input column is already a DictionaryArray, the writer throws that structure away before encoding it. downcast_dict_op! hands a TypedDictionaryArray to encode, which only sees T: ArrayAccessor — so values.value(idx) flattens key → value, and DictEncoder::encode interns once per row. That re-derives, for every row, a mapping the input dictionary already carried: an ahash of the value bytes, a hash table probe, and a byte comparison against the dictionary page. I found this profiling compaction in GreptimeDB, where mito2 stores each SST's primary key as a Dictionary(UInt32, Binary) column. Rows are sorted by primary key, so keys arrive in long runs — and 82% of the time spent encoding that column was in Interner::intern, re-hashing values the dictionary had already deduplicated. # What changes are included in this PR? downcast_op! takes a second op, and the Dictionary arms dispatch to it. The new encode_dict keeps the keys visible and resolves each one through a small direct-mapped cache from input dictionary key to interned key. A hit is one comparison and no hashing; the slot also stores the value length so the loop maintains variable_length_bytes without touching the dictionary at all. The cache is a fixed 24 KiB regardless of dictionary size, which is what lets the lookup be unconditional. I tried an exact per-dictionary remap table first and it needed a size heuristic to avoid regressing high-cardinality columns — a fixed cache degrades on its own instead: columns whose keys repeat hit nearly always, columns with no repetition miss and pay one predictable integer compare per row. Slots are generation-stamped so a new call invalidates the cache without clearing it. That's required, not an optimization — a key only means anything relative to one input dictionary, and consecutive batches can carry different ones. Duplicate and unused dictionary entries stay correct: duplicates intern to the same key, unused entries are never interned. Statistics and bloom filter updates moved into a shared helper so the two entry points can't drift. Scope: only Dictionary(_, Utf8 | LargeUtf8 | Binary | LargeBinary | FixedSizeBinary) inputs. All other byte array types keep the existing path, and if parquet dictionary encoding is off the fallback encoder is used as before. # Are these changes tested? Existing tests cover the round trips. Added arrow_writer_dictionary_key_cache_handles_collisions_and_rebinding for the two ways a direct-mapped cache can go wrong: keys colliding in the same slot, and a later batch binding the same key to a different value. Removing either the slot tag check or the generation check makes it fail. arrow_writer benchmarks, 1,048,576 rows per batch. Running two binaries alternately rather than --save-baseline, because this machine drifted ~13% on string_dictionary/zstd over half an hour and the first comparison I did was mostly measuring that: string_dictionary_low_cardinality_20/default -47% / -42% string_dictionary_low_cardinality_20/zstd -39% / -45% string_dictionary_low_cardinality_100/default -36% / -41% string_dictionary_low_cardinality_400/default -21% .. -31% string_dictionary/default -3.3% .. -4.9% string_dictionary/zstd -1.5% .. -2.2% string_dictionary/bloom_filter -0.5% / +1.9% string_dictionary is the high-cardinality case, where the cache mostly misses — it's there to show the miss path doesn't cost anything, not as a win. # Are there any user-facing changes? no -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
