emilk commented on code in PR #10759:
URL: https://github.com/apache/arrow-rs/pull/10759#discussion_r3815954421
##########
arrow-flight/src/sql/metadata/sql_info.rs:
##########
@@ -356,11 +356,8 @@ impl SqlInfoDataBuilder {
let mut name_builder = UInt32Builder::new();
let mut value_builder = SqlInfoUnionBuilder::new();
- let mut names: Vec<_> = self.infos.keys().copied().collect();
- names.sort_unstable();
-
- for key in names {
- let (name, value) = self.infos.get_key_value(&key).unwrap();
+ // `infos` is a `BTreeMap`, so it iterates in sorted order already
+ for (name, value) in &self.infos {
Review Comment:
nice little speed-up
##########
arrow-buffer/src/pool.rs:
##########
@@ -147,6 +147,17 @@ impl MemoryReservation for Tracker {
}
}
+/// Lock a memory reservation, recovering from a poisoned lock.
+///
+/// A poisoned lock only means that some other thread panicked. The
reservation it
+/// guards is plain size accounting, so there is no broken invariant to
protect, and
+/// recovering it is always preferable to panicking.
+pub(crate) fn lock_reservation(
+ reservation: &Mutex<Option<Box<dyn MemoryReservation>>>,
+) -> MutexGuard<'_, Option<Box<dyn MemoryReservation>>> {
+ reservation.lock().unwrap_or_else(PoisonError::into_inner)
+}
Review Comment:
Maybe a bit weird. Alternatives to consider:
* Do a single `unwrap()` here and just document crate-wide that poisoning
causes panics
* Use `parking_lot`s non-poisoning mutexes
--
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]
