Skip to content

Commit

Permalink
Remove unsafe Send impl from PriorityMap (apache#12289)
Browse files Browse the repository at this point in the history
It's not necessary to use unsafe Send impl. It's enough to require the
referenced trait objects as Send.
  • Loading branch information
findepi authored Sep 3, 2024
1 parent 4e1b6de commit 4a227c5
Show file tree
Hide file tree
Showing 3 changed files with 11 additions and 9 deletions.
5 changes: 4 additions & 1 deletion datafusion/physical-plan/src/aggregates/topk/hash_table.rs
Original file line number Diff line number Diff line change
Expand Up @@ -367,7 +367,10 @@ has_integer!(u8, u16, u32, u64);
has_integer!(IntervalDayTime, IntervalMonthDayNano);
hash_float!(f16, f32, f64);

pub fn new_hash_table(limit: usize, kt: DataType) -> Result<Box<dyn ArrowHashTable>> {
pub fn new_hash_table(
limit: usize,
kt: DataType,
) -> Result<Box<dyn ArrowHashTable + Send>> {
macro_rules! downcast_helper {
($kt:ty, $d:ident) => {
return Ok(Box::new(PrimitiveHashTable::<$kt>::new(limit)))
Expand Down
6 changes: 5 additions & 1 deletion datafusion/physical-plan/src/aggregates/topk/heap.rs
Original file line number Diff line number Diff line change
Expand Up @@ -459,7 +459,11 @@ compare_integer!(u8, u16, u32, u64);
compare_integer!(IntervalDayTime, IntervalMonthDayNano);
compare_float!(f16, f32, f64);

pub fn new_heap(limit: usize, desc: bool, vt: DataType) -> Result<Box<dyn ArrowHeap>> {
pub fn new_heap(
limit: usize,
desc: bool,
vt: DataType,
) -> Result<Box<dyn ArrowHeap + Send>> {
macro_rules! downcast_helper {
($vt:ty, $d:ident) => {
return Ok(Box::new(PrimitiveHeap::<$vt>::new(limit, desc, vt)))
Expand Down
9 changes: 2 additions & 7 deletions datafusion/physical-plan/src/aggregates/topk/priority_map.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,17 +25,12 @@ use datafusion_common::Result;

/// A `Map<K, V>` / `PriorityQueue` combo that evicts the worst values after reaching `capacity`
pub struct PriorityMap {
map: Box<dyn ArrowHashTable>,
heap: Box<dyn ArrowHeap>,
map: Box<dyn ArrowHashTable + Send>,
heap: Box<dyn ArrowHeap + Send>,
capacity: usize,
mapper: Vec<(usize, usize)>,
}

// JUSTIFICATION
// Benefit: ~15% speedup + required to index into RawTable from binary heap
// Soundness: it is only accessed by one thread at a time, and indexes are kept up to date
unsafe impl Send for PriorityMap {}

impl PriorityMap {
pub fn new(
key_type: DataType,
Expand Down

0 comments on commit 4a227c5

Please sign in to comment.