clean up final

Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>

Author: connortsui20

vortex-btrblocks/src/schemes/float.rs

@@ -74,14 +74,6 @@ impl rle::RLEConfig for FloatRLEConfig {
     fn generate_stats(array: &ArrayRef) -> FloatStats {
         FloatStats::generate(&array.to_primitive())
     }
-
-    fn compress_values(
-        compressor: &CascadingCompressor,
-        values: &PrimitiveArray,
-        ctx: CompressorContext,
-    ) -> VortexResult<ArrayRef> {
-        compressor.compress_canonical(Canonical::Primitive(values.clone()), ctx)
-    }
 }
 
 impl RLEStats for FloatStats {

vortex-btrblocks/src/schemes/integer.rs

@@ -98,14 +98,6 @@ impl rle::RLEConfig for IntRLEConfig {
     fn generate_stats(array: &ArrayRef) -> IntegerStats {
         IntegerStats::generate(&array.to_primitive())
     }
-
-    fn compress_values(
-        compressor: &CascadingCompressor,
-        values: &PrimitiveArray,
-        ctx: CompressorContext,
-    ) -> VortexResult<ArrayRef> {
-        compressor.compress_canonical(Canonical::Primitive(values.clone()), ctx)
-    }
 }
 
 impl RLEStats for IntegerStats {

vortex-btrblocks/src/schemes/rle.rs

@@ -38,8 +38,6 @@ pub trait RLEStats {
     fn source(&self) -> &PrimitiveArray;
 }
 
-// TODO(connor): This trait is super confusing, we should probably just remove it and hardcode the
-// only 2 implementations (integer and float).
 /// Configuration trait for RLE schemes.
 ///
 /// Implement this trait to define the behavior of an RLE scheme for a specific
@@ -56,13 +54,6 @@ pub trait RLEConfig: Debug + Send + Sync + 'static {
 
     /// Generates statistics for the given array.
     fn generate_stats(array: &ArrayRef) -> Self::Stats;
-
-    /// Compress the values array after RLE encoding.
-    fn compress_values(
-        compressor: &CascadingCompressor,
-        values: &PrimitiveArray,
-        ctx: CompressorContext,
-    ) -> VortexResult<ArrayRef>;
 }
 
 /// RLE scheme that is generic over a configuration type.

vortex-compressor/public-api.lock

@@ -402,14 +402,6 @@ pub fn vortex_compressor::ctx::CompressorContext::fmt(&self, f: &mut core::fmt::
 
 pub const vortex_compressor::ctx::MAX_CASCADE: usize
 
-pub mod vortex_compressor::root_list_children
-
-pub const vortex_compressor::root_list_children::ELEMENTS: usize
-
-pub const vortex_compressor::root_list_children::OFFSETS: usize
-
-pub const vortex_compressor::root_list_children::SIZES: usize
-
 pub mod vortex_compressor::scheme
 
 pub enum vortex_compressor::scheme::ChildSelection
@@ -1040,8 +1032,6 @@ impl vortex_compressor::CascadingCompressor
 
 pub fn vortex_compressor::CascadingCompressor::compress(&self, array: &vortex_array::array::ArrayRef) -> vortex_error::VortexResult<vortex_array::array::ArrayRef>
 
-pub fn vortex_compressor::CascadingCompressor::compress_canonical(&self, array: vortex_array::canonical::Canonical, ctx: vortex_compressor::ctx::CompressorContext) -> vortex_error::VortexResult<vortex_array::array::ArrayRef>
-
 pub fn vortex_compressor::CascadingCompressor::compress_child(&self, child: &vortex_array::array::ArrayRef, parent_ctx: &vortex_compressor::ctx::CompressorContext, parent_id: vortex_compressor::scheme::SchemeId, child_index: usize) -> vortex_error::VortexResult<vortex_array::array::ArrayRef>
 
 pub fn vortex_compressor::CascadingCompressor::execution_ctx(&self) -> parking_lot::mutex::MutexGuard<'_, vortex_array::executor::ExecutionCtx>

vortex-compressor/src/compressor.rs

@@ -47,9 +47,7 @@ const ROOT_SCHEME_ID: SchemeId = SchemeId {
 };
 
 /// Child indices for the compressor's list/listview compression.
-pub mod root_list_children {
-    /// List elements child.
-    pub const ELEMENTS: usize = 0;
+mod root_list_children {
     /// List/ListView offsets child.
     pub const OFFSETS: usize = 1;
     /// ListView sizes child.
@@ -166,7 +164,7 @@ impl CascadingCompressor {
     /// # Errors
     ///
     /// Returns an error if compression of any sub-array fails.
-    pub fn compress_canonical(
+    fn compress_canonical(
         &self,
         array: Canonical,
         ctx: CompressorContext,
@@ -326,12 +324,23 @@ impl CascadingCompressor {
         let mut best: Option<(&'static dyn Scheme, f64)> = None;
 
         for &scheme in schemes {
+            // Constant detection on a sample is a false positive: the sample being constant
+            // does not mean the full array is constant.
+            if ctx.is_sample() && scheme.detects_constant() {
+                continue;
+            }
+
             let ratio = scheme.expected_compression_ratio(self, data, ctx.clone())?;
 
             tracing::debug!(scheme = %scheme.id(), ratio, "evaluated compression ratio");
 
             if is_better_ratio(ratio, &best) {
                 best = Some((scheme, ratio));
+
+                // Schemes that return f64::MAX (like Constant) cannot be beat, so stop early.
+                if ratio == f64::MAX {
+                    break;
+                }
             }
         }
 
@@ -450,3 +459,79 @@ impl CascadingCompressor {
 fn is_better_ratio(ratio: f64, best: &Option<(&'static dyn Scheme, f64)>) -> bool {
     ratio.is_finite() && !ratio.is_subnormal() && ratio > 1.0 && best.is_none_or(|(_, r)| ratio > r)
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use crate::builtins::FloatDictScheme;
+    use crate::builtins::IntDictScheme;
+    use crate::builtins::IntUncompressedScheme;
+    use crate::builtins::StringDictScheme;
+    use crate::ctx::CompressorContext;
+    use crate::scheme::SchemeExt;
+
+    fn compressor() -> CascadingCompressor {
+        CascadingCompressor::new(vec![
+            &IntUncompressedScheme,
+            &IntDictScheme,
+            &FloatDictScheme,
+            &StringDictScheme,
+        ])
+    }
+
+    #[test]
+    fn test_self_exclusion() {
+        let c = compressor();
+        let ctx = CompressorContext::default().descend_with_scheme(IntDictScheme.id(), 0);
+
+        // IntDictScheme is in the history, so it should be excluded.
+        assert!(c.is_excluded(&IntDictScheme, &ctx));
+        // IntUncompressedScheme is not in the history.
+        assert!(!c.is_excluded(&IntUncompressedScheme, &ctx));
+    }
+
+    #[test]
+    fn test_root_exclusion_list_offsets() {
+        let c = compressor();
+        let ctx = CompressorContext::default()
+            .descend_with_scheme(ROOT_SCHEME_ID, root_list_children::OFFSETS);
+
+        // IntDict should be excluded for list offsets.
+        assert!(c.is_excluded(&IntDictScheme, &ctx));
+        // IntUncompressed should not be excluded.
+        assert!(!c.is_excluded(&IntUncompressedScheme, &ctx));
+    }
+
+    #[test]
+    fn test_push_rule_float_dict_excludes_int_dict_from_codes() {
+        let c = compressor();
+        // FloatDict cascading through codes (child 1).
+        let ctx = CompressorContext::default().descend_with_scheme(FloatDictScheme.id(), 1);
+
+        // IntDict should be excluded from FloatDict's codes child.
+        assert!(c.is_excluded(&IntDictScheme, &ctx));
+        // IntUncompressed should not be excluded.
+        assert!(!c.is_excluded(&IntUncompressedScheme, &ctx));
+    }
+
+    #[test]
+    fn test_push_rule_float_dict_excludes_int_dict_from_values() {
+        let c = compressor();
+        // FloatDict cascading through values (child 0).
+        let ctx = CompressorContext::default().descend_with_scheme(FloatDictScheme.id(), 0);
+
+        // IntDict should also be excluded from FloatDict's values child (ALP propagation
+        // replacement).
+        assert!(c.is_excluded(&IntDictScheme, &ctx));
+    }
+
+    #[test]
+    fn test_no_exclusion_without_history() {
+        let c = compressor();
+        let ctx = CompressorContext::default();
+
+        // No history means no exclusions.
+        assert!(!c.is_excluded(&IntDictScheme, &ctx));
+        assert!(!c.is_excluded(&IntUncompressedScheme, &ctx));
+    }
+}

vortex-compressor/src/lib.rs

@@ -25,4 +25,3 @@ mod sample;
 
 mod compressor;
 pub use compressor::CascadingCompressor;
-pub use compressor::root_list_children;