kosiew commented on code in PR #24105:
URL: https://github.com/apache/datafusion/pull/24105#discussion_r3977413639
##########
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 there is a correctness issue here. `id: None` now represents both a
NULL group key and a freed store slot. Because this filter requires
`item.id.is_some()`, an occupied NULL-key group whose MIN/MAX input is entirely
NULL gets registered but is skipped here. That means `PriorityMap::emit` can
omit the group entirely.
Could we preserve an explicit occupied/vacant state, or use a distinct
sentinel for vacant slots, so an occupied NULL key remains distinguishable from
a freed slot? It would also be good to add a regression test with a NULL group
key and an all-NULL aggregate.
##########
datafusion/physical-plan/src/aggregates/topk/heap.rs:
##########
@@ -776,4 +759,6 @@ mod tests {
Ok(())
}
+
+ // TODO: test TopKHeap of String?
Review Comment:
Could we replace this TODO with a focused `TopKHeap<String>` test? Filling
the heap with distinct string values and then exercising replacement and drain
would give us direct coverage of the borrowed-string comparison and
owned-string replacement introduced by this change.
--
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]
