kosiew commented on code in PR #19285:
URL: https://github.com/apache/datafusion/pull/19285#discussion_r2663733616
##########
datafusion/physical-plan/src/aggregates/topk/heap.rs:
##########
@@ -161,6 +225,166 @@ where
}
}
+/// An implementation of `ArrowHeap` that deals with string values.
+///
+/// Supports all three UTF-8 string types: `Utf8`, `LargeUtf8`, and `Utf8View`.
+/// String values are compared lexicographically. Null values are not
explicitly handled
+/// and should not appear in the input; the aggregation layer ensures nulls
are managed
+/// appropriately before calling this heap.
+///
+/// Uses string interning to avoid repeated allocations for duplicate strings
within a batch.
+/// The `string_cache` maps string hashes to `Arc<str>` values, amortizing
allocation costs
+/// when the same string appears multiple times (common in trace IDs, user
IDs, etc.).
+pub struct StringHeap {
+ batch: ArrayRef,
+ heap: TopKHeap<Arc<str>>,
+ desc: bool,
+ data_type: DataType,
+ /// Cache of interned strings for the current batch, mapping hash to
`Arc<str>`.
+ /// Cleared on each `set_batch` call to prevent memory leaks from old
batches.
+ string_cache: HashMap<u64, Arc<str>>,
Review Comment:
After getting bad benchmark results, I refactored this from
per-batch string interning cache (mapping hashes to `Arc<str>`)
to
compare-first design that stores `Option<String>` in the heap
This removes the need for string_cache too.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
