SubhamSinghal commented on code in PR #27136:
URL: https://github.com/apache/datafusion/pull/27136#discussion_r4501542410
##########
datafusion/physical-plan/src/topk/mod.rs:
##########
@@ -2621,110 +2626,84 @@ impl PartitionedTopKRank {
}
}
-/// A run of rows from a single source [`RecordBatch`] sharing one
-/// distinct ORDER BY value. Materialized at emit time via
-/// [`take_record_batch`].
-///
-/// The batch is referenced by `batch_id` rather than held directly, so the
-/// operator-scoped [`RecordBatchStore`] can charge each distinct source
-/// batch once however many entries reference it. Holding a batch per entry
-/// and charging its bytes per entry would inflate the reservation by a
-/// factor of (partitions × K), since a single batch contributes an entry to
-/// every partition and ob group it touches.
-#[derive(Debug)]
-struct GroupEntry {
- /// Indices into the batch identified by `batch_id`. Always non-empty
- /// by construction.
- row_indices: Vec<u32>,
- /// Key into `PartitionedTopKDenseRank::store`.
- batch_id: u32,
-}
-
/// Per-partition state for `DENSE_RANK()` semantics.
///
-/// A `HashMap<Vec<u8>, Vec<GroupEntry>>` keyed by the row-encoded ORDER
-/// BY bytes, capped at `k` distinct keys. Each key's `Vec<GroupEntry>`
-/// holds every row seen at that ob value, one entry per contributing
-/// source `RecordBatch`.
-#[derive(Default)]
+/// Every retained row of the partition, grouped by ORDER BY value: `groups`
+/// maps each of the at most `k` distinct-smallest values seen so far to where
+/// every row holding it lives in the store. Like [`RankPartitionState`], this
+/// holds only store coordinates: the output columns of every retained row live
+/// in the operator's one shared [`RecordBatchStore`].
+#[derive(Debug, Default)]
struct DenseRankPartitionState {
- groups: HashMap<Vec<u8>, Vec<GroupEntry>>,
+ /// Keyed by the row-encoded ORDER BY bytes, capped at `k` keys. Each
+ /// group's rows are in arrival order, which `emit` keeps.
+ groups: HashMap<Vec<u8>, Vec<StoreRef>>,
Review Comment:
I tried using BTreeMap but seeing regression at high cardinality. Maybe I
can spend some more time on this as followup.
--
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]
