[CALCITE-4593] DiffRepository tests should fail if new XML resources …

…are not in alphabetical order

If a test uses a resource that occurs out of order,
DiffRepository writes out the log file in sorted order, then
fails the test, indicating that the problem can be fixed by
copying the log file over the reference file.

If a test has a different output (say, a plan has changed),
DiffRepository writes out the actual results to the log file,
as today. Files are always written in sorted order.

This commit sorts all existing resource files, and converts
the XML indentation to 2. This destroys history, and will
probably make it impossible to merge existing PRs, but we
only need to do this once.

Close apache#2409

.editorconfig

@@ -48,3 +48,6 @@ ij_java_ternary_operation_signs_on_next_line = true
 ij_java_use_single_class_imports = true
 ij_java_wrap_long_lines = true
 ij_java_align_multiline_parameters = false
+
+[*.xml]
+indent_size = 2

core/src/test/java/org/apache/calcite/test/DiffRepository.java

@@ -25,6 +25,7 @@
 import com.google.common.cache.CacheBuilder;
 import com.google.common.cache.CacheLoader;
 import com.google.common.cache.LoadingCache;
+import com.google.common.collect.ImmutableSortedSet;
 
 import org.junit.jupiter.api.Assertions;
 import org.opentest4j.AssertionFailedError;
@@ -42,11 +43,12 @@
 import java.io.IOException;
 import java.io.Writer;
 import java.net.URL;
+import java.util.AbstractList;
 import java.util.ArrayList;
-import java.util.HashSet;
 import java.util.List;
 import java.util.Objects;
-import java.util.Set;
+import java.util.SortedMap;
+import java.util.TreeMap;
 import javax.xml.parsers.DocumentBuilder;
 import javax.xml.parsers.DocumentBuilderFactory;
 import javax.xml.parsers.ParserConfigurationException;
@@ -171,8 +173,10 @@ public class DiffRepository {
 
   private final DiffRepository baseRepository;
   private final int indent;
+  private final ImmutableSortedSet<String> outOfOrderTests;
   private Document doc;
   private final Element root;
+  private final URL refFile;
   private final File logFile;
   private final Filter filter;
 
@@ -183,17 +187,17 @@ public class DiffRepository {
    * @param logFile   Log file
    * @param baseRepository Parent repository or null
    * @param filter    Filter or null
+   * @param indent    Indentation of XML file
    */
-  private DiffRepository(
-      URL refFile,
-      File logFile,
-      DiffRepository baseRepository,
-      Filter filter) {
+  private DiffRepository(URL refFile, File logFile,
+      DiffRepository baseRepository, Filter filter, int indent) {
     this.baseRepository = baseRepository;
     this.filter = filter;
+    this.indent = indent;
     if (refFile == null) {
       throw new IllegalArgumentException("url must not be null");
     }
+    this.refFile = refFile;
     this.logFile = logFile;
 
     // Load the document.
@@ -213,13 +217,10 @@ private DiffRepository(
         flushDoc();
       }
       this.root = doc.getDocumentElement();
-      validate(this.root);
+      outOfOrderTests = validate(this.root);
     } catch (ParserConfigurationException | SAXException e) {
       throw new RuntimeException("error while creating xml parser", e);
     }
-    indent = logFile.getPath().contains("RelOptRulesTest")
-        || logFile.getPath().contains("SqlToRelConverterTest")
-        || logFile.getPath().contains("SqlLimitsTest") ? 4 : 2;
   }
 
   //~ Methods ----------------------------------------------------------------
@@ -373,6 +374,13 @@ private synchronized Element getTestCaseElement(
                 + "test case in the base repository, but does "
                 + "not specify 'overrides=true'");
           }
+          if (outOfOrderTests.contains(testCaseName)) {
+            flushDoc();
+            throw new IllegalArgumentException("TestCase '" + testCaseName
+                + "' is out of order in the reference file: "
+                + Sources.of(refFile).file() + "\n"
+                + "To fix, copy the generated log file: " + logFile + "\n");
+          }
           return testCase;
         }
         if (elements != null) {
@@ -530,26 +538,48 @@ private void flushDoc() {
     }
   }
 
-  /** Validates the root element. */
-  private static void validate(Element root) {
+  /** Validates the root element.
+   *
+   * <p>Returns the set of test names that are out of order in the reference
+   * file (empty if the reference file is fully sorted). */
+  private static ImmutableSortedSet<String> validate(Element root) {
     if (!root.getNodeName().equals(ROOT_TAG)) {
       throw new RuntimeException("expected root element of type '" + ROOT_TAG
           + "', but found '" + root.getNodeName() + "'");
     }
 
-    // Make sure that there are no duplicate test cases.
-    final Set<String> testCases = new HashSet<>();
+    // Make sure that there are no duplicate test cases, and count how many
+    // tests are out of order.
+    final SortedMap<String, Node> testCases = new TreeMap<>();
     final NodeList childNodes = root.getChildNodes();
+    final List<String> outOfOrderNames = new ArrayList<>();
+    String previousName = null;
     for (int i = 0; i < childNodes.getLength(); i++) {
       Node child = childNodes.item(i);
       if (child.getNodeName().equals(TEST_CASE_TAG)) {
         Element testCase = (Element) child;
         final String name = testCase.getAttribute(TEST_CASE_NAME_ATTR);
-        if (!testCases.add(name)) {
+        if (testCases.put(name, testCase) != null) {
           throw new RuntimeException("TestCase '" + name + "' is duplicate");
         }
+        if (previousName != null
+            && previousName.compareTo(name) > 0) {
+          outOfOrderNames.add(name);
+        }
+        previousName = name;
+      }
+    }
+
+    // If any nodes were out of order, rebuild the document in sorted order.
+    if (!outOfOrderNames.isEmpty()) {
+      for (Node testCase : testCases.values()) {
+        root.removeChild(testCase);
+      }
+      for (Node testCase : testCases.values()) {
+        root.appendChild(testCase);
       }
     }
+    return ImmutableSortedSet.copyOf(outOfOrderNames);
   }
 
   /**
@@ -629,9 +659,6 @@ private static void writeNode(Node node, XmlOutput out) {
         Node child = childNodes.item(i);
         writeNode(child, out);
       }
-
-      //            writeNode(((Document) node).getDocumentElement(),
-      // out);
       break;
 
     case Node.ELEMENT_NODE:
@@ -719,20 +746,19 @@ public static DiffRepository lookup(Class<?> clazz) {
     return lookup(clazz, null);
   }
 
-  /**
-   * Finds the repository instance for a given class and inheriting from
-   * a given repository.
-   *
-   * @param clazz     Test case class
-   * @param baseRepository Base class of test class
-   * @return The diff repository shared between test cases in this class.
-   */
+  @Deprecated // to be removed before 1.28
   public static DiffRepository lookup(
       Class<?> clazz,
       DiffRepository baseRepository) {
     return lookup(clazz, baseRepository, null);
   }
 
+  @Deprecated // to be removed before 1.28
+  public static DiffRepository lookup(Class<?> clazz,
+      DiffRepository baseRepository, Filter filter) {
+    return lookup(clazz, baseRepository, filter, 2);
+  }
+
   /**
    * Finds the repository instance for a given class.
    *
@@ -756,12 +782,13 @@ public static DiffRepository lookup(
    * @param clazz     Test case class
    * @param baseRepository Base repository
    * @param filter    Filters each string returned by the repository
-   * @return The diff repository shared between test cases in this class.
+   * @param indent    Indent of the XML file (usually 2)
+   *
+   * @return The diff repository shared between test cases in this class
    */
   public static DiffRepository lookup(Class<?> clazz,
-      DiffRepository baseRepository,
-      Filter filter) {
-    final Key key = new Key(clazz, baseRepository, filter);
+      DiffRepository baseRepository, Filter filter, int indent) {
+    final Key key = new Key(clazz, baseRepository, filter, indent);
     return REPOSITORY_CACHE.getUnchecked(key);
   }
 
@@ -792,11 +819,14 @@ private static class Key {
     private final Class<?> clazz;
     private final DiffRepository baseRepository;
     private final Filter filter;
+    private final int indent;
 
-    Key(Class<?> clazz, DiffRepository baseRepository, Filter filter) {
+    Key(Class<?> clazz, DiffRepository baseRepository, Filter filter,
+        int indent) {
       this.clazz = Objects.requireNonNull(clazz, "clazz");
       this.baseRepository = baseRepository;
       this.filter = filter;
+      this.indent = indent;
     }
 
     @Override public int hashCode() {
@@ -817,7 +847,20 @@ DiffRepository toRepo() {
       final String logFilePath = refFilePath.replace(".xml", "_actual.xml");
       final File logFile = new File(logFilePath);
       assert !refFilePath.equals(logFile.getAbsolutePath());
-      return new DiffRepository(refFile, logFile, baseRepository, filter);
+      return new DiffRepository(refFile, logFile, baseRepository, filter,
+          indent);
     }
   }
+
+  private static Iterable<Node> iterate(NodeList nodeList) {
+    return new AbstractList<Node>() {
+      @Override public Node get(int index) {
+        return nodeList.item(index);
+      }
+
+      @Override public int size() {
+        return nodeList.getLength();
+      }
+    };
+  }
 }

core/src/test/java/org/apache/calcite/util/TestUtil.java

@@ -18,13 +18,16 @@
 
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Suppliers;
+import com.google.common.collect.ImmutableSortedSet;
 
 import org.junit.jupiter.api.Assertions;
 
 import java.io.PrintWriter;
 import java.io.StringWriter;
 import java.lang.reflect.InvocationTargetException;
+import java.util.List;
 import java.util.Objects;
+import java.util.SortedSet;
 import java.util.function.Supplier;
 import java.util.regex.Matcher;
 import java.util.regex.Pattern;
@@ -298,6 +301,20 @@ private static int computeGuavaMajorVersion() {
         .bestVersion;
   }
 
+  /** Given a list, returns the number of elements that are not between an
+   * element that is less and an element that is greater. */
+  public static <E extends Comparable<E>> SortedSet<E> outOfOrderItems(List<E> list) {
+    E previous = null;
+    final ImmutableSortedSet.Builder<E> b = ImmutableSortedSet.naturalOrder();
+    for (E e : list) {
+      if (previous != null && previous.compareTo(e) > 0) {
+        b.add(e);
+      }
+      previous = e;
+    }
+    return b.build();
+  }
+
   /** Checks if exceptions have give substring. That is handy to prevent logging SQL text twice */
   public static boolean hasMessage(Throwable t, String substring) {
     while (t != null) {

core/src/test/java/org/apache/calcite/util/TestUtilTest.java

@@ -16,8 +16,18 @@
  */
 package org.apache.calcite.util;
 
+import org.apache.calcite.util.mapping.IntPair;
+
+import com.google.common.collect.ImmutableMap;
+
 import org.junit.jupiter.api.Test;
 
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.List;
+import java.util.SortedSet;
+import java.util.stream.Collectors;
+
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.hamcrest.core.Is.is;
 import static org.junit.jupiter.api.Assertions.assertEquals;
@@ -122,4 +132,33 @@ private void testJavaVersion(int expectedMajorVersion, String versionString) {
     // can't handle nines that start right after the point very well. oh well.
     assertThat(TestUtil.correctRoundedFloat("12.999999"), is("12."));
   }
+
+  /** Tests {@link TestUtil#outOfOrderItems(List)}. */
+  @Test void testOutOfOrderItems() {
+    final List<String> list =
+        new ArrayList<>(Arrays.asList("a", "g", "b", "c", "e", "d"));
+    final SortedSet<String> distance = TestUtil.outOfOrderItems(list);
+    assertThat(distance.toString(), is("[b, d]"));
+
+    list.add("f");
+    final SortedSet<String> distance2 = TestUtil.outOfOrderItems(list);
+    assertThat(distance2.toString(), is("[b, d]"));
+
+    list.add(1, "b");
+    final SortedSet<String> distance3 = TestUtil.outOfOrderItems(list);
+    assertThat(distance3.toString(), is("[b, d]"));
+
+    list.add(1, "c");
+    final SortedSet<String> distance4 = TestUtil.outOfOrderItems(list);
+    assertThat(distance4.toString(), is("[b, d]"));
+
+    list.add(0, "z");
+    final SortedSet<String> distance5 = TestUtil.outOfOrderItems(list);
+    assertThat(distance5.toString(), is("[a, b, d]"));
+  }
+
+  private long totalDistance(ImmutableMap<String, IntPair> map) {
+    return map.entrySet().stream()
+        .collect(Collectors.summarizingInt(e -> e.getValue().target)).getSum();
+  }
 }