DiffRepository should write a test's resource file only when it is modified
Before this change, DiffRepository writes the resource file
(to xxx_actual.xml) for every test case, regardless of whether
its resources (e.g. SQL or plan) have changed. For large tests
with lots of test cases and large resource files, such as
RelOptRulesTest, that is a considerable CPU and IO cost.
After this change, DiffRepository only writes the resource
file if the resources change. So if RelOptRulesTest has 3
failures out of 500 tests, DiffRepository will write
RelOptRulesTest_actual.xml 3 times. As a result, such tests
run ~10x faster.
There are a few test methods in SqlToRelConverter test that
run 2 or 3 SQL statements that all produce the same plan.
DiffRepository regards each SQL statement as a resource, and
marks the resource file 'dirty' even though the final result
is always the same. We should fix this, but it's only a minor
impact on performance.
diff --git a/core/src/main/java/org/apache/calcite/util/Util.java b/core/src/main/java/org/apache/calcite/util/Util.java
index cd4fc0f..fe771cb 100644
--- a/core/src/main/java/org/apache/calcite/util/Util.java
+++ b/core/src/main/java/org/apache/calcite/util/Util.java
@@ -967,6 +967,9 @@ public static RuntimeException throwAsRuntime(String message, Throwable throwabl
throwable.addSuppressed(new Throwable(message));
throw (Error) throwable;
}
+ if (throwable instanceof IOException) {
+ return new UncheckedIOException(message, (IOException) throwable);
+ }
throw new RuntimeException(message, throwable);
}
diff --git a/core/src/test/java/org/apache/calcite/test/DiffRepository.java b/core/src/test/java/org/apache/calcite/test/DiffRepository.java
index 65c2529..b1c010b 100644
--- a/core/src/test/java/org/apache/calcite/test/DiffRepository.java
+++ b/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.ImmutableList;
import com.google.common.collect.ImmutableSortedSet;
import org.junit.jupiter.api.Assertions;
@@ -179,6 +180,8 @@ public class DiffRepository {
private final URL refFile;
private final File logFile;
private final Filter filter;
+ private int modCount;
+ private int modCountAtLastWrite;
/**
* Creates a DiffRepository.
@@ -194,11 +197,10 @@ private DiffRepository(URL refFile, File logFile,
this.baseRepository = baseRepository;
this.filter = filter;
this.indent = indent;
- if (refFile == null) {
- throw new IllegalArgumentException("url must not be null");
- }
- this.refFile = refFile;
+ this.refFile = Objects.requireNonNull(refFile, "refFile");
this.logFile = logFile;
+ this.modCountAtLastWrite = 0;
+ this.modCount = 0;
// Load the document.
DocumentBuilderFactory fac = DocumentBuilderFactory.newInstance();
@@ -375,6 +377,7 @@ private synchronized Element getTestCaseElement(
+ "not specify 'overrides=true'");
}
if (outOfOrderTests.contains(testCaseName)) {
+ ++modCount;
flushDoc();
throw new IllegalArgumentException("TestCase '" + testCaseName
+ "' is out of order in the reference file: "
@@ -471,6 +474,7 @@ private synchronized void update(
testCaseElement.setAttribute(TEST_CASE_NAME_ATTR, testCaseName);
Node refElement = ref(testCaseName, map);
root.insertBefore(testCaseElement, refElement);
+ ++modCount;
}
Element resourceElement =
getResourceElement(testCaseElement, resourceName, true);
@@ -478,11 +482,20 @@ private synchronized void update(
resourceElement = doc.createElement(RESOURCE_TAG);
resourceElement.setAttribute(RESOURCE_NAME_ATTR, resourceName);
testCaseElement.appendChild(resourceElement);
+ ++modCount;
+ if (!value.equals("")) {
+ resourceElement.appendChild(doc.createCDATASection(value));
+ }
} else {
- removeAllChildren(resourceElement);
- }
- if (!value.equals("")) {
- resourceElement.appendChild(doc.createCDATASection(value));
+ final List<Node> newChildList;
+ if (value.equals("")) {
+ newChildList = ImmutableList.of();
+ } else {
+ newChildList = ImmutableList.of(doc.createCDATASection(value));
+ }
+ if (replaceChildren(resourceElement, newChildList)) {
+ ++modCount;
+ }
}
// Write out the document.
@@ -525,17 +538,23 @@ private Node ref(String testCaseName, List<Pair<String, Element>> map) {
/**
* Flushes the reference document to the file system.
*/
- private void flushDoc() {
+ private synchronized void flushDoc() {
+ if (modCount == modCountAtLastWrite) {
+ // Document has not been modified since last write.
+ return;
+ }
try {
boolean b = logFile.getParentFile().mkdirs();
Util.discard(b);
try (Writer w = Util.printWriter(logFile)) {
write(doc, w, indent);
}
+ System.out.println("write; modCount=" + modCount);
} catch (IOException e) {
- throw new RuntimeException("error while writing test reference log '"
+ throw Util.throwAsRuntime("error while writing test reference log '"
+ logFile + "'", e);
}
+ modCountAtLastWrite = modCount;
}
/** Validates the root element.
@@ -636,6 +655,34 @@ private static void removeAllChildren(Element element) {
}
}
+ private static boolean replaceChildren(Element element, List<Node> children) {
+ // Current children
+ final NodeList childNodes = element.getChildNodes();
+ final List<Node> list = new ArrayList<>();
+ for (Node item : iterate(childNodes)) {
+ if (item.getNodeType() != Node.TEXT_NODE) {
+ list.add(item);
+ }
+ }
+
+ // Are new children equal to old?
+ if (equalList(children, list)) {
+ return false;
+ }
+
+ // Replace old children with new children
+ removeAllChildren(element);
+ children.forEach(element::appendChild);
+ return true;
+ }
+
+ /** Returns whether two lists of nodes are equal. */
+ private static boolean equalList(List<Node> list0, List<Node> list1) {
+ return list1.size() == list0.size()
+ && Pair.zip(list1, list0).stream()
+ .allMatch(p -> p.left.isEqualNode(p.right));
+ }
+
/**
* Serializes an XML document as text.
*
