Retrofit comparator suites to capture-not-abort; default mode shadow (PR 2/7)

src/test/java/com/databricks/jdbc/comparator/error/Captures.java

@@ -1,6 +1,7 @@
 package com.databricks.jdbc.comparator.error;
 
 import com.databricks.jdbc.comparator.ComparisonResult;
+import java.sql.ResultSet;
 import java.util.function.Function;
 
 /**
@@ -29,6 +30,34 @@ public static CapturedOutcome capture(JdbcCall call) {
     }
   }
 
+  /**
+   * Runs a driver call and returns its result on success, or the thrown {@link Throwable} itself on
+   * failure — so the value can be handed directly to {@link
+   * com.databricks.jdbc.comparator.ResultSetComparator#compare}, which dispatches on runtime type
+   * (ResultSet vs Throwable) and, when the error gate is on, compares thrown errors deeply.
+   *
+   * <p>Wrap ONLY the driver call; keep provider bookkeeping outside so harness bugs still
+   * propagate.
+   */
+  public static Object resultOrThrowable(JdbcCall call) {
+    try {
+      return call.call();
+    } catch (Throwable t) {
+      return t;
+    }
+  }
+
+  /** Closes the value if it is a ResultSet; ignores nulls, non-ResultSets, and close errors. */
+  public static void closeIfResultSet(Object value) {
+    if (value instanceof ResultSet) {
+      try {
+        ((ResultSet) value).close();
+      } catch (Exception ignored) {
+        // best-effort cleanup
+      }
+    }
+  }
+
   /**
    * Compares two captured outcomes and folds the result into a {@link ComparisonResult}. Error
    * diffs land in the metadata/data difference lists in the exact formats {@code csvSummary()}

src/test/java/com/databricks/jdbc/comparator/error/ErrorComparatorTest.java

@@ -7,6 +7,7 @@
 
 import com.databricks.jdbc.comparator.ComparisonResult;
 import java.sql.SQLException;
+import java.util.List;
 import org.junit.jupiter.api.Test;
 
 /** Unit tests for the pure error capture/compare logic (no JDBC connection required). */
@@ -134,13 +135,80 @@ void offModeDisablesDeepComparison() {
   }
 
   @Test
-  void unrecognizedModeDefaultsToOffInsteadOfThrowing() {
+  void unrecognizedModeDefaultsToShadowInsteadOfThrowing() {
     String prev = System.getProperty("ERROR_COMPARISON_MODE");
     try {
       System.setProperty("ERROR_COMPARISON_MODE", "shaddow"); // typo
       ErrorPolicy p = ErrorPolicy.active();
-      assertEquals(ErrorPolicy.Mode.OFF, p.mode());
-      assertFalse(p.isDeepComparisonEnabled());
+      // Fail safe: a bad value must not throw. It falls back to the default (shadow), which is
+      // itself CI-safe (records DIFF rows, never fails the build).
+      assertEquals(ErrorPolicy.Mode.SHADOW, p.mode());
+      assertTrue(p.isDeepComparisonEnabled());
+    } finally {
+      if (prev == null) {
+        System.clearProperty("ERROR_COMPARISON_MODE");
+      } else {
+        System.setProperty("ERROR_COMPARISON_MODE", prev);
+      }
+    }
+  }
+
+  @Test
+  void unsetModeDefaultsToShadow() {
+    String prev = System.getProperty("ERROR_COMPARISON_MODE");
+    try {
+      System.clearProperty("ERROR_COMPARISON_MODE");
+      assertEquals(ErrorPolicy.Mode.SHADOW, ErrorPolicy.active().mode());
+    } finally {
+      if (prev != null) {
+        System.setProperty("ERROR_COMPARISON_MODE", prev);
+      }
+    }
+  }
+
+  // ---- ErrorDiffs gate (used by DML/Volume providers) ----
+
+  @Test
+  void errorDiffsOffModeDoesLegacyClassOnly() {
+    withMode(
+        "off",
+        () -> {
+          // Same class, different SQLState/message -> OFF must NOT emit a SQLState/message diff.
+          CapturedOutcome l = CapturedOutcome.threw(new SQLException("a", "42P01", 1));
+          CapturedOutcome r = CapturedOutcome.threw(new SQLException("b", "08000", 2));
+          assertTrue(ErrorDiffs.compare(l, r, "v ").isEmpty());
+        });
+  }
+
+  @Test
+  void errorDiffsShadowModeComparesDeeply() {
+    withMode(
+        "shadow",
+        () -> {
+          CapturedOutcome l = CapturedOutcome.threw(new SQLException("a", "42P01", 1));
+          CapturedOutcome r = CapturedOutcome.threw(new SQLException("b", "08000", 2));
+          List<String> diffs = ErrorDiffs.compare(l, r, "v ");
+          assertTrue(diffs.stream().anyMatch(d -> d.contains("SQLState mismatch")));
+        });
+  }
+
+  @Test
+  void errorDiffsOffModeStillFlagsDifferingClass() {
+    withMode(
+        "off",
+        () -> {
+          CapturedOutcome l = CapturedOutcome.threw(new SQLException("a", "42P01", 1));
+          CapturedOutcome r = CapturedOutcome.threw(new java.sql.SQLDataException("b", "42P01", 1));
+          List<String> diffs = ErrorDiffs.compare(l, r, "v ");
+          assertTrue(diffs.stream().anyMatch(d -> d.contains("exception type mismatch")));
+        });
+  }
+
+  private static void withMode(String mode, Runnable body) {
+    String prev = System.getProperty("ERROR_COMPARISON_MODE");
+    try {
+      System.setProperty("ERROR_COMPARISON_MODE", mode);
+      body.run();
     } finally {
       if (prev == null) {
         System.clearProperty("ERROR_COMPARISON_MODE");

src/test/java/com/databricks/jdbc/comparator/error/ErrorDiffs.java

@@ -0,0 +1,56 @@
+package com.databricks.jdbc.comparator.error;
+
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+
+/**
+ * Bridges suite providers that capture their own {@link CapturedOutcome}s (DML, Volume) to the
+ * error comparison, while honoring the {@code ERROR_COMPARISON_MODE} gate the same way {@code
+ * ResultSetComparator.compareErrorsDeeply} does — so {@code off} is a true kill switch everywhere.
+ *
+ * <p>When the gate is on, compares errors deeply (class + SQLState + code + message). When off,
+ * falls back to the minimal legacy check (exception class only) so no SQLState/code/message diffs
+ * are emitted.
+ */
+public final class ErrorDiffs {
+
+  private ErrorDiffs() {}
+
+  /**
+   * Returns diff strings for a pair of outcomes where at least one threw. Empty when they agree
+   * (per the active mode). {@code returnedLabel} prefixes how a returned value is rendered in a
+   * one-sided diff (e.g. {@code "update count "}).
+   */
+  public static List<String> compare(
+      CapturedOutcome left, CapturedOutcome right, String returnedLabel) {
+    ErrorPolicy policy = ErrorPolicy.active();
+    if (!policy.isDeepComparisonEnabled()) {
+      return legacyClassOnly(left, right);
+    }
+    ErrorComparison c = ErrorComparator.compare(left, right, v -> returnedLabel + v);
+    List<String> diffs = new ArrayList<>(c.metadataDiffs);
+    diffs.addAll(c.dataDiffs);
+    return diffs;
+  }
+
+  /**
+   * Legacy behavior when the gate is off: both threw → flag only a differing exception class;
+   * one-sided → flag which side threw. No SQLState/code/message comparison.
+   */
+  private static List<String> legacyClassOnly(CapturedOutcome left, CapturedOutcome right) {
+    if (left.threw() && right.threw()) {
+      String l = left.error().simpleClassName();
+      String r = right.error().simpleClassName();
+      return l.equals(r)
+          ? Collections.emptyList()
+          : Collections.singletonList("exception type mismatch: " + l + " vs " + r);
+    }
+    if (left.threw()) {
+      return Collections.singletonList(
+          "left threw " + left.error().simpleClassName() + " but right returned");
+    }
+    return Collections.singletonList(
+        "right threw " + right.error().simpleClassName() + " but left returned");
+  }
+}

src/test/java/com/databricks/jdbc/comparator/error/ErrorPolicy.java

@@ -5,7 +5,9 @@
  *
  * <p>Modes advance with the rollout: {@code off} (legacy class-only check) → {@code shadow}
  * (compare and report, never fail CI) → {@code authoritative} (un-baselined error diffs fail the
- * build). Read the active policy via {@link #active()}.
+ * build). The default is {@code shadow}: deep comparison runs and records DIFF rows, but a DIFF row
+ * does not fail the build (only a re-thrown Throwable does), so the default is CI-safe. Read the
+ * active policy via {@link #active()}.
  *
  * <p>Message normalization and tolerance/baseline handling are intentionally omitted for now — we
  * compare the raw fields (class, SQLState, vendor code, message) and will add tolerance later, from
@@ -21,14 +23,20 @@ public enum Mode {
 
   private static final String MODE_PROPERTY = "ERROR_COMPARISON_MODE";
 
+  /**
+   * Default when the flag is unset, empty, or unrecognized. Shadow is CI-safe (records, never
+   * fails). Advanced to {@code authoritative} in the final rollout PR.
+   */
+  private static final Mode DEFAULT_MODE = Mode.SHADOW;
+
   private final Mode mode;
 
   private ErrorPolicy(Mode mode) {
     this.mode = mode;
   }
 
   /**
-   * Resolves the active policy from system properties. Defaults to {@link Mode#OFF} for null,
+   * Resolves the active policy from system properties. Defaults to {@link #DEFAULT_MODE} for null,
    * empty, or unrecognized values (the latter logs a warning) so a misconfigured flag never aborts
    * a comparison run.
    */
@@ -42,7 +50,7 @@ public static ErrorPolicy of(Mode mode) {
 
   private static Mode parseMode(String raw) {
     if (raw == null || raw.isEmpty()) {
-      return Mode.OFF;
+      return DEFAULT_MODE;
     }
     switch (raw.trim().toLowerCase()) {
       case "shadow":
@@ -52,13 +60,15 @@ private static Mode parseMode(String raw) {
       case "off":
         return Mode.OFF;
       default:
-        // Fail safe: a typo in the flag must not abort the comparison run. Default to OFF
-        // (legacy behavior) and warn, rather than throwing from every compare() call.
+        // Fail safe: a typo in the flag must not abort the comparison run. Fall back to the
+        // default mode and warn, rather than throwing from every compare() call.
         System.err.println(
             "[comparator] Unknown ERROR_COMPARISON_MODE '"
                 + raw
-                + "' (expected off|shadow|authoritative); defaulting to off.");
-        return Mode.OFF;
+                + "' (expected off|shadow|authoritative); defaulting to "
+                + DEFAULT_MODE.name().toLowerCase()
+                + ".");
+        return DEFAULT_MODE;
     }
   }
 

src/test/java/com/databricks/jdbc/comparator/suite/PreparedStatementTypesProvider.java

@@ -2,6 +2,7 @@
 
 import com.databricks.jdbc.comparator.ComparisonResult;
 import com.databricks.jdbc.comparator.ResultSetComparator;
+import com.databricks.jdbc.comparator.error.Captures;
 import java.math.BigDecimal;
 import java.sql.*;
 import java.util.Arrays;
@@ -183,10 +184,17 @@ public ComparisonResult execute(
       test.setter.set(ps1);
       test.setter.set(ps2);
 
-      try (ResultSet rs1 = ps1.executeQuery();
-          ResultSet rs2 = ps2.executeQuery()) {
-        assertCloudFetchExpectation(testCase, rs1, rs2);
-        return ResultSetComparator.compare(label, sql, testCase.getArgs(), rs1, rs2);
+      // Capture executeQuery per side so a one-sided/divergent error is compared, not aborted.
+      Object r1 = Captures.resultOrThrowable(ps1::executeQuery);
+      Object r2 = Captures.resultOrThrowable(ps2::executeQuery);
+      try {
+        if (r1 instanceof ResultSet && r2 instanceof ResultSet) {
+          assertCloudFetchExpectation(testCase, (ResultSet) r1, (ResultSet) r2);
+        }
+        return ResultSetComparator.compare(label, sql, testCase.getArgs(), r1, r2);
+      } finally {
+        Captures.closeIfResultSet(r1);
+        Captures.closeIfResultSet(r2);
       }
     }
   }

src/test/java/com/databricks/jdbc/comparator/suite/StatementDmlProvider.java

@@ -2,8 +2,12 @@
 
 import com.databricks.jdbc.comparator.ComparisonResult;
 import com.databricks.jdbc.comparator.ResultSetComparator;
+import com.databricks.jdbc.comparator.error.CapturedOutcome;
+import com.databricks.jdbc.comparator.error.Captures;
+import com.databricks.jdbc.comparator.error.ErrorDiffs;
 import java.sql.Connection;
 import java.sql.ResultSet;
+import java.sql.SQLException;
 import java.sql.Statement;
 import java.util.ArrayList;
 import java.util.Collections;
@@ -114,44 +118,32 @@ public ComparisonResult execute(
     return result;
   }
 
-  /** Executes DML on both connections and compares update counts and exceptions. */
+  /** Executes DML on both connections and compares update counts and errors. */
   private void compareDml(
       Connection conn1,
       Connection conn2,
       String sql1,
       String sql2,
       List<String> differences,
       String operation) {
-    Object result1 = executeDml(conn1, sql1);
-    Object result2 = executeDml(conn2, sql2);
-
-    if (result1 instanceof Exception && result2 instanceof Exception) {
-      String type1 = result1.getClass().getSimpleName();
-      String type2 = result2.getClass().getSimpleName();
-      if (!type1.equals(type2)) {
-        differences.add(operation + ": exception type mismatch: " + type1 + " vs " + type2);
+    CapturedOutcome left = Captures.capture(() -> executeDml(conn1, sql1));
+    CapturedOutcome right = Captures.capture(() -> executeDml(conn2, sql2));
+
+    if (left.threw() || right.threw()) {
+      // An error on either side. Honor the ERROR_COMPARISON_MODE gate so `off` disables deep
+      // comparison here just as it does on the ResultSetComparator path (the rollout kill switch).
+      for (String d : ErrorDiffs.compare(left, right, "update count ")) {
+        differences.add(operation + ": " + d);
       }
-    } else if (result1 instanceof Exception) {
+    } else if (!left.value().equals(right.value())) {
       differences.add(
-          operation
-              + ": Thrift threw "
-              + result1.getClass().getSimpleName()
-              + " but SEA succeeded");
-    } else if (result2 instanceof Exception) {
-      differences.add(
-          operation + ": Thrift succeeded but SEA threw " + result2.getClass().getSimpleName());
-    } else {
-      if (!result1.equals(result2)) {
-        differences.add(operation + ": update count mismatch: " + result1 + " vs " + result2);
-      }
+          operation + ": update count mismatch: " + left.value() + " vs " + right.value());
     }
   }
 
-  private Object executeDml(Connection conn, String sql) {
+  private int executeDml(Connection conn, String sql) throws SQLException {
     try (Statement stmt = conn.createStatement()) {
       return stmt.executeUpdate(sql);
-    } catch (Exception e) {
-      return e;
     }
   }
 

src/test/java/com/databricks/jdbc/comparator/suite/StatementOtherProvider.java

@@ -2,6 +2,7 @@
 
 import com.databricks.jdbc.comparator.ComparisonResult;
 import com.databricks.jdbc.comparator.ResultSetComparator;
+import com.databricks.jdbc.comparator.error.Captures;
 import java.sql.Connection;
 import java.sql.ResultSet;
 import java.sql.Statement;
@@ -71,16 +72,22 @@ public ComparisonResult execute(
     } else if (isExecuteOnly(sql)) {
       try (Statement s1 = conn1.createStatement();
           Statement s2 = conn2.createStatement()) {
-        Object result1 = s1.execute(sql);
-        Object result2 = s2.execute(sql);
+        // execute() returns a Boolean; a thrown error is captured and compared, not aborted.
+        Object result1 = Captures.resultOrThrowable(() -> s1.execute(sql));
+        Object result2 = Captures.resultOrThrowable(() -> s2.execute(sql));
         return ResultSetComparator.compare(label, sql, testCase.getArgs(), result1, result2);
       }
     } else {
       try (Statement s1 = conn1.createStatement();
-          Statement s2 = conn2.createStatement();
-          ResultSet rs1 = s1.executeQuery(sql);
-          ResultSet rs2 = s2.executeQuery(sql)) {
-        return ResultSetComparator.compare(label, sql, testCase.getArgs(), rs1, rs2);
+          Statement s2 = conn2.createStatement()) {
+        Object r1 = Captures.resultOrThrowable(() -> s1.executeQuery(sql));
+        Object r2 = Captures.resultOrThrowable(() -> s2.executeQuery(sql));
+        try {
+          return ResultSetComparator.compare(label, sql, testCase.getArgs(), r1, r2);
+        } finally {
+          Captures.closeIfResultSet(r1);
+          Captures.closeIfResultSet(r2);
+        }
       }
     }
   }