Skip to content

Conversation

@Harrm
Copy link

@Harrm Harrm commented Nov 12, 2025

  • Add support for custom column comparators in RocksDb
  • Expose DBRawIterator for seek operations

@Harrm Harrm marked this pull request as ready for review November 21, 2025 11:41
pub memory_budget: Option<MiB>,

/// Custom comparison function for row ordering
pub comparator: Option<std::sync::Arc<dyn Comparator<'static>>>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this complicated dance necessary? Arc + creator function? Why not just pass in the function directly and copy it below?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is necessary because rocksdb wants a Box (which is not cloneable), but this function is also stored in ColumnConfig (which is cloneable). So I need both a way to obtain a Box as a comparator, and have it in some cloneable entity. Hence an Arc'ed factory to produce boxed comparators.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I mean is the following, which seems to work just fine:

diff --git a/kvdb-rocksdb/src/lib.rs b/kvdb-rocksdb/src/lib.rs
index cd6f780..74910de 100644
--- a/kvdb-rocksdb/src/lib.rs
+++ b/kvdb-rocksdb/src/lib.rs
@@ -146,31 +146,6 @@ impl CompactionProfile {
 	}
 }
 
-/// Custom comparison function for row ordering
-pub trait Comparator<'a>: Send + Sync {
-	fn get_fn(&self) -> Box<dyn Fn(&[u8], &[u8]) -> cmp::Ordering + 'a>;
-}
-
-/// A wrapper to allow the comparator to be both cloneable and convertible into Box<dyn Fn>,
-/// which is required by RocksDb
-pub struct ComparatorWrapper<T> {
-	cmp: T,
-}
-
-impl<T> ComparatorWrapper<T> {
-	pub fn new(cmp: T) -> Self {
-		Self { cmp }
-	}
-}
-
-impl<T: 'static + Send + Sync + Clone + Fn(&[u8], &[u8]) -> cmp::Ordering> Comparator<'static>
-	for ComparatorWrapper<T>
-{
-	fn get_fn(&self) -> Box<dyn Fn(&[u8], &[u8]) -> cmp::Ordering + 'static> {
-		Box::new(self.cmp.clone())
-	}
-}
-
 #[derive(Clone, Default)]
 pub struct ColumnConfig {
 	/// Memory budget (in MiB) used for setting block cache size and
@@ -185,7 +160,8 @@ pub struct ColumnConfig {
 	/// The client must ensure that the comparator supplied here has the same
 	/// name and orders keys *exactly* the same as the comparator provided to
 	/// previous open calls on the same DB.
-	pub comparator: Option<std::sync::Arc<dyn Comparator<'static>>>,
+	/// Custom comparison function for row ordering
+	pub comparator: Option<fn(&[u8], &[u8]) -> cmp::Ordering>,
 }
 
 /// Database configuration
@@ -282,8 +258,8 @@ impl DatabaseConfig {
 		opts.optimize_level_style_compaction(column_mem_budget);
 		opts.set_target_file_size_base(self.compaction.initial_file_size);
 		opts.set_compression_per_level(&[]);
-		if let Some(comparator) = &self.columns[col as usize].comparator {
-			opts.set_comparator(&format!("column_{col}_comparator"), comparator.get_fn());
+		if let Some(comparator) = self.columns[col as usize].comparator {
+			opts.set_comparator(&format!("column_{col}_comparator"), Box::new(comparator));
 		}
 
 		opts

opts.set_target_file_size_base(self.compaction.initial_file_size);
opts.set_compression_per_level(&[]);
if let Some(comparator) = &self.columns[col as usize].comparator {
opts.set_comparator(&format!("column_{col}_comparator"), comparator.get_fn());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or simply (with suggested changes above): opts.set_comparator(&format ..., Box::new(comparator));

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants