kosiew commented on code in PR #24105:
URL: https://github.com/apache/datafusion/pull/24105#discussion_r4004085780
##########
datafusion/physical-plan/src/aggregates/topk/hash_table.rs:
##########
@@ -482,9 +498,7 @@ impl<ID: KeyType + PartialEq> TopKHashTable<ID> {
.iter()
.enumerate()
.filter_map(|(idx, item)| {
- item.as_ref()
- .filter(|item| item.heap_idx == NULL_HEAP_IDX)
- .map(|_| idx)
+ (item.id.is_some() && item.is_null()).then_some(idx)
Review Comment:
I think the NULL-key issue is still present here. `HashTableItem::id ==
None` can mean either a valid NULL grouping key or a vacant store slot, but
this filter only keeps entries where `item.id.is_some()`.
That means a NULL-key group registered through `insert_null` can still be
skipped by `PriorityMap::emit`. In particular, if the MIN/MAX input for that
group is entirely NULL, the group can disappear from the aggregate output.
You mentioned iterating `map` instead of filtering `store`, which seems like
a good way to avoid overloading `id` as the occupancy marker. Otherwise, I
think we need a separate occupied/vacant state or a distinct vacancy sentinel.
Could you also add a regression test with a NULL group key and an all-NULL
aggregate input? That should make this behavior explicit.
##########
datafusion/physical-plan/src/aggregates/topk/heap.rs:
##########
@@ -776,4 +759,6 @@ mod tests {
Ok(())
}
+
+ // TODO: test TopKHeap of String?
Review Comment:
Could we add the focused `TopKHeap<String>` regression test here before
removing this TODO? It would be useful to cover distinct string inserts,
replacement of an existing heap entry, and draining the heap. That should
directly exercise the borrowed comparison path together with owned-value
replacement.
--
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]
