clean up scoring logic

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

Author: connortsui20

vortex-compressor/src/compressor.rs

@@ -400,7 +400,7 @@ impl CascadingCompressor {
                 EstimateVerdict::Skip => {}
                 EstimateVerdict::AlwaysUse => return Ok(Some((scheme, WinnerEstimate::AlwaysUse))),
                 EstimateVerdict::Ratio(ratio) => {
-                    let score = EstimateScore::Finite(ratio);
+                    let score = EstimateScore::FiniteCompression(ratio);
                     if is_better_score(score, &best) {
                         best = Some((scheme, score));
                     }
@@ -1069,7 +1069,7 @@ mod tests {
 
         assert!(matches!(
             winner,
-            Some((scheme, WinnerEstimate::Score(EstimateScore::Finite(2.0))))
+            Some((scheme, WinnerEstimate::Score(EstimateScore::FiniteCompression(2.0))))
                 if scheme.id() == DirectRatioScheme.id()
         ));
         Ok(())
@@ -1086,7 +1086,7 @@ mod tests {
 
         assert!(matches!(
             winner,
-            Some((scheme, WinnerEstimate::Score(EstimateScore::Finite(3.0))))
+            Some((scheme, WinnerEstimate::Score(EstimateScore::FiniteCompression(3.0))))
                 if scheme.id() == CallbackRatioScheme.id()
         ));
         Ok(())
@@ -1109,6 +1109,23 @@ mod tests {
         Ok(())
     }
 
+    #[test]
+    fn finite_ratio_does_not_displace_zero_byte_sample() -> VortexResult<()> {
+        let compressor = CascadingCompressor::new(vec![&ZeroBytesSamplingScheme, &HugeRatioScheme]);
+        let schemes: [&'static dyn Scheme; 2] = [&ZeroBytesSamplingScheme, &HugeRatioScheme];
+        let mut data = estimate_test_data();
+
+        let winner =
+            compressor.choose_best_scheme(&schemes, &mut data, CompressorContext::new())?;
+
+        assert!(matches!(
+            winner,
+            Some((scheme, WinnerEstimate::Score(EstimateScore::ZeroBytes)))
+                if scheme.id() == ZeroBytesSamplingScheme.id()
+        ));
+        Ok(())
+    }
+
     #[test]
     fn all_null_array_compresses_to_constant() -> VortexResult<()> {
         let array = PrimitiveArray::new(
@@ -1151,7 +1168,7 @@ mod tests {
         //   "this must be present since `DictScheme` declared that we need distinct values"
         let score =
             estimate_compression_ratio_with_sampling(&FloatDictScheme, &compressor, &array, ctx)?;
-        assert!(matches!(score, EstimateScore::Finite(ratio) if ratio.is_finite()));
+        assert!(matches!(score, EstimateScore::FiniteCompression(ratio) if ratio.is_finite()));
         Ok(())
     }
 

vortex-compressor/src/estimate.rs

@@ -89,9 +89,13 @@ pub enum DeferredEstimate {
 /// Ranked estimate used for comparing non-terminal compression candidates.
 #[derive(Debug, Clone, Copy, PartialEq)]
 pub(super) enum EstimateScore {
-    /// A finite compression ratio.
-    Finite(f64),
+    /// A finite compression ratio. Higher means a smaller amount of data, so it is better.
+    FiniteCompression(f64),
     /// Trial compression produced a 0-byte output.
+    ///
+    /// This is treated as an unbounded score for scheme selection, but has no finite trace ratio.
+    /// Final full-array compression still has to pass the normal before/after acceptance check, so
+    /// a zero-byte sample can choose a scheme but cannot force a larger output to be returned.
     ZeroBytes,
 }
 
@@ -101,17 +105,39 @@ impl EstimateScore {
         if after_nbytes == 0 {
             Self::ZeroBytes
         } else {
-            Self::Finite(before_nbytes as f64 / after_nbytes as f64)
+            Self::FiniteCompression(before_nbytes as f64 / after_nbytes as f64)
         }
     }
 
     /// Returns the traceable numeric ratio, omitting the zero-byte special case.
     pub(super) fn trace_ratio(self) -> Option<f64> {
         match self {
-            Self::Finite(ratio) => Some(ratio),
+            Self::FiniteCompression(ratio) => Some(ratio),
             Self::ZeroBytes => None,
         }
     }
+
+    /// Returns whether this estimate is eligible to compete.
+    fn is_valid(self) -> bool {
+        match self {
+            Self::FiniteCompression(ratio) => {
+                ratio.is_finite() && !ratio.is_subnormal() && ratio > 1.0
+            }
+            Self::ZeroBytes => true,
+        }
+    }
+
+    /// Returns whether this estimate beats another valid estimate.
+    fn beats(self, other: Self) -> bool {
+        match (self, other) {
+            (Self::ZeroBytes, Self::FiniteCompression(_)) => true,
+            (Self::ZeroBytes, Self::ZeroBytes) => false,
+            (Self::FiniteCompression(_), Self::ZeroBytes) => false,
+            (Self::FiniteCompression(ratio), Self::FiniteCompression(best_ratio)) => {
+                ratio > best_ratio
+            }
+        }
+    }
 }
 
 /// Winner estimate carried from scheme selection into result tracing.
@@ -138,20 +164,7 @@ pub(super) fn is_better_score(
     score: EstimateScore,
     best: &Option<(&'static dyn Scheme, EstimateScore)>,
 ) -> bool {
-    match score {
-        EstimateScore::ZeroBytes => {
-            best.is_none_or(|(_, best_score)| !matches!(best_score, EstimateScore::ZeroBytes))
-        }
-        EstimateScore::Finite(ratio) => {
-            ratio.is_finite()
-                && !ratio.is_subnormal()
-                && ratio > 1.0
-                && best.is_none_or(|(_, best_score)| match best_score {
-                    EstimateScore::Finite(best_ratio) => ratio > best_ratio,
-                    EstimateScore::ZeroBytes => false,
-                })
-        }
-    }
+    score.is_valid() && best.is_none_or(|(_, best_score)| score.beats(best_score))
 }
 
 /// Estimates compression ratio by compressing a ~1% sample of the data.