revert scoring logic

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

Author: connortsui20

vortex-compressor/src/compressor.rs

@@ -1093,7 +1093,7 @@ mod tests {
     }
 
     #[test]
-    fn zero_byte_sample_beats_any_finite_ratio() -> VortexResult<()> {
+    fn zero_byte_sample_loses_to_finite_ratio() -> VortexResult<()> {
         let compressor = CascadingCompressor::new(vec![&HugeRatioScheme, &ZeroBytesSamplingScheme]);
         let schemes: [&'static dyn Scheme; 2] = [&HugeRatioScheme, &ZeroBytesSamplingScheme];
         let mut data = estimate_test_data();
@@ -1103,14 +1103,14 @@ mod tests {
 
         assert!(matches!(
             winner,
-            Some((scheme, WinnerEstimate::Score(EstimateScore::ZeroBytes)))
-                if scheme.id() == ZeroBytesSamplingScheme.id()
+            Some((scheme, WinnerEstimate::Score(EstimateScore::FiniteCompression(100.0))))
+                if scheme.id() == HugeRatioScheme.id()
         ));
         Ok(())
     }
 
     #[test]
-    fn finite_ratio_does_not_displace_zero_byte_sample() -> VortexResult<()> {
+    fn finite_ratio_displaces_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();
@@ -1120,12 +1120,25 @@ mod tests {
 
         assert!(matches!(
             winner,
-            Some((scheme, WinnerEstimate::Score(EstimateScore::ZeroBytes)))
-                if scheme.id() == ZeroBytesSamplingScheme.id()
+            Some((scheme, WinnerEstimate::Score(EstimateScore::FiniteCompression(100.0))))
+                if scheme.id() == HugeRatioScheme.id()
         ));
         Ok(())
     }
 
+    #[test]
+    fn zero_byte_sample_alone_selects_no_scheme() -> VortexResult<()> {
+        let compressor = CascadingCompressor::new(vec![&ZeroBytesSamplingScheme]);
+        let schemes: [&'static dyn Scheme; 1] = [&ZeroBytesSamplingScheme];
+        let mut data = estimate_test_data();
+
+        let winner =
+            compressor.choose_best_scheme(&schemes, &mut data, CompressorContext::new())?;
+
+        assert!(winner.is_none());
+        Ok(())
+    }
+
     #[test]
     fn all_null_array_compresses_to_constant() -> VortexResult<()> {
         let array = PrimitiveArray::new(
@@ -1220,7 +1233,7 @@ mod tests {
     }
 
     #[test]
-    fn zero_byte_results_omit_ratio_fields() {
+    fn zero_byte_sample_result_omits_ratio_fields_and_selects_no_scheme() {
         let compressor = CascadingCompressor::new(vec![&ZeroBytesSamplingScheme]);
         let array = test_integer_array();
 
@@ -1235,12 +1248,12 @@ mod tests {
         );
         assert!(!sample_event.fields.contains_key("sampled_ratio"));
 
-        let result_event = find_event(&events, TARGET_TRACE, "scheme.compress_result");
-        assert_eq!(
-            result_event.fields.get("after_nbytes").map(String::as_str),
-            Some("0")
-        );
-        assert!(!result_event.fields.contains_key("estimated_ratio"));
-        assert!(!result_event.fields.contains_key("actual_ratio"));
+        assert!(!events.iter().any(|event| {
+            event.target == TARGET_TRACE
+                && event
+                    .fields
+                    .get("message")
+                    .is_some_and(|value| value == "scheme.compress_result")
+        }));
     }
 }

vortex-compressor/src/estimate.rs

@@ -93,9 +93,10 @@ pub(super) enum EstimateScore {
     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.
+    /// This has no finite trace ratio and is not eligible for scheme selection.
+    ///
+    /// TODO(connor): A zero-byte sample usually means the sampler happened to hit an all-null
+    /// sample. Improve this logic so we can distinguish real zero-byte wins from sampling artifacts.
     ZeroBytes,
 }
 
@@ -123,16 +124,15 @@ impl EstimateScore {
             Self::FiniteCompression(ratio) => {
                 ratio.is_finite() && !ratio.is_subnormal() && ratio > 1.0
             }
-            Self::ZeroBytes => true,
+            Self::ZeroBytes => false,
         }
     }
 
     /// 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::ZeroBytes, _) => false,
+            (Self::FiniteCompression(_), Self::ZeroBytes) => true,
             (Self::FiniteCompression(ratio), Self::FiniteCompression(best_ratio)) => {
                 ratio > best_ratio
             }