Handle metrics for the same query but with different setups (#3531)

Planner metrics previously assumed that it was only reasonable to have a
query once within a single block. Having setup can make this more
complicated, so this changes the keys for the metrics to have the setup
too.

There were a couple unrelated cleanups along the way:
1. `markDirty()` was annotated `@Nullable`, but is `void`
2. `putMetrics` took `isDirectyMetrics`, but that parameter was never
used, so I removed it.

Author: ScottDugas

yaml-tests/src/main/java/com/apple/foundationdb/relational/yamltests/YamlExecutionContext.java

@@ -179,11 +179,10 @@ public synchronized PlannerMetricsProto.Info putMetrics(@Nonnull final String bl
                                                             @Nonnull final String query,
                                                             final int lineNumber,
                                                             @Nonnull final PlannerMetricsProto.Info info,
-                                                            boolean isDirtyMetrics) {
-        return actualMetricsMap.put(new QueryAndLocation(blockName, query, lineNumber), info);
+                                                            @Nonnull final List<String> setups) {
+        return actualMetricsMap.put(new QueryAndLocation(blockName, query, lineNumber, setups), info);
     }
 
-    @Nullable
     @SuppressWarnings("UnusedReturnValue")
     public synchronized void markDirty() {
         this.isDirtyMetrics = true;
@@ -388,10 +387,7 @@ public void saveMetricsAsBinaryProto() {
         final var condensedMetricsMap = new LinkedHashMap<PlannerMetricsProto.Identifier, PlannerMetricsProto.Info>();
         for (final var entry : actualMetricsMap.entrySet()) {
             final var queryAndLocation = entry.getKey();
-            final var identifier = PlannerMetricsProto.Identifier.newBuilder()
-                    .setBlockName(queryAndLocation.getBlockName())
-                    .setQuery(queryAndLocation.getQuery())
-                    .build();
+            final var identifier = queryAndLocation.getIdentifier();
             if (condensedMetricsMap.containsKey(identifier)) {
                 logger.warn("⚠️ Repeated query in block {} at line {}", queryAndLocation.getBlockName(),
                         queryAndLocation.getLineNumber());
@@ -423,20 +419,25 @@ public void saveMetricsAsYaml() {
 
         final var mmap = LinkedListMultimap.<String, Map<String, Object>>create();
         for (final var entry : actualMetricsMap.entrySet()) {
-            final var identifier = entry.getKey();
+            final var identifier = entry.getKey().getIdentifier();
             final var info = entry.getValue();
             final var countersAndTimers = info.getCountersAndTimers();
-            final var infoMap =
-                    ImmutableMap.<String, Object>of("query", identifier.getQuery(),
-                            "explain", info.getExplain(),
-                            "task_count", countersAndTimers.getTaskCount(),
-                            "task_total_time_ms", TimeUnit.NANOSECONDS.toMillis(countersAndTimers.getTaskTotalTimeNs()),
-                            "transform_count", countersAndTimers.getTransformCount(),
-                            "transform_time_ms", TimeUnit.NANOSECONDS.toMillis(countersAndTimers.getTransformTimeNs()),
-                            "transform_yield_count", countersAndTimers.getTransformYieldCount(),
-                            "insert_time_ms", TimeUnit.NANOSECONDS.toMillis(countersAndTimers.getInsertTimeNs()),
-                            "insert_new_count", countersAndTimers.getInsertNewCount(),
-                            "insert_reused_count", countersAndTimers.getInsertReusedCount());
+            final var infoMap = new LinkedHashMap<String, Object>();
+            infoMap.put("query", identifier.getQuery());
+            // only include setup if it is non-empty, in part so that the PR that adds setup doesn't change every
+            // metric in the yaml files
+            if (identifier.getSetupsCount() > 0) {
+                infoMap.put("setup", identifier.getSetupsList());
+            }
+            infoMap.put("explain", info.getExplain());
+            infoMap.put("task_count", countersAndTimers.getTaskCount());
+            infoMap.put("task_total_time_ms", TimeUnit.NANOSECONDS.toMillis(countersAndTimers.getTaskTotalTimeNs()));
+            infoMap.put("transform_count", countersAndTimers.getTransformCount());
+            infoMap.put("transform_time_ms", TimeUnit.NANOSECONDS.toMillis(countersAndTimers.getTransformTimeNs()));
+            infoMap.put("transform_yield_count", countersAndTimers.getTransformYieldCount());
+            infoMap.put("insert_time_ms", TimeUnit.NANOSECONDS.toMillis(countersAndTimers.getInsertTimeNs()));
+            infoMap.put("insert_new_count", countersAndTimers.getInsertNewCount());
+            infoMap.put("insert_reused_count", countersAndTimers.getInsertReusedCount());
             mmap.put(identifier.getBlockName(), infoMap);
         }
 
@@ -513,23 +514,31 @@ private static String baseName(@Nonnull final String resourcePath) {
 
     private static class QueryAndLocation {
         @Nonnull
-        private final String blockName;
-        private final String query;
+        private final PlannerMetricsProto.Identifier identifier;
         private final int lineNumber;
 
-        public QueryAndLocation(@Nonnull final String blockName, final String query, final int lineNumber) {
-            this.blockName = blockName;
-            this.query = query;
+        public QueryAndLocation(@Nonnull final String blockName, final String query, final int lineNumber,
+                                @Nonnull List<String> setups) {
+            identifier = PlannerMetricsProto.Identifier.newBuilder()
+                    .setBlockName(blockName)
+                    .setQuery(query)
+                    .addAllSetups(setups)
+                    .build();
             this.lineNumber = lineNumber;
         }
 
+        @Nonnull
+        public PlannerMetricsProto.Identifier getIdentifier() {
+            return identifier;
+        }
+
         @Nonnull
         public String getBlockName() {
-            return blockName;
+            return identifier.getBlockName();
         }
 
         public String getQuery() {
-            return query;
+            return identifier.getQuery();
         }
 
         public int getLineNumber() {
@@ -542,12 +551,12 @@ public boolean equals(final Object o) {
                 return false;
             }
             final QueryAndLocation that = (QueryAndLocation)o;
-            return lineNumber == that.lineNumber && Objects.equals(blockName, that.blockName) && Objects.equals(query, that.query);
+            return lineNumber == that.lineNumber && Objects.equals(identifier, that.identifier);
         }
 
         @Override
         public int hashCode() {
-            return Objects.hash(blockName, query, lineNumber);
+            return Objects.hash(identifier, lineNumber);
         }
     }
 

yaml-tests/src/main/java/com/apple/foundationdb/relational/yamltests/command/QueryConfig.java

@@ -134,9 +134,9 @@ String decorateQuery(@Nonnull String query) {
     }
 
     final void checkResult(@Nonnull String currentQuery, @Nonnull Object actual, @Nonnull String queryDescription,
-                           @Nonnull YamlConnection connection) {
+                           @Nonnull YamlConnection connection, @Nonnull List<String> setups) {
         try {
-            checkResultInternal(currentQuery, actual, queryDescription);
+            checkResultInternal(currentQuery, actual, queryDescription, setups);
         } catch (AssertionFailedError e) {
             throw executionContext.wrapContext(e,
                     () -> "‼️Check result failed in config at line " + getLineNumber() + " against connection for versions " + connection.getVersions(),
@@ -163,7 +163,7 @@ final void checkError(@Nonnull SQLException actual, @Nonnull String queryDescrip
     }
 
     abstract void checkResultInternal(@Nonnull String currentQuery, @Nonnull Object actual,
-                                      @Nonnull String queryDescription) throws SQLException;
+                                      @Nonnull String queryDescription, @Nonnull List<String> setups) throws SQLException;
 
     void checkErrorInternal(@Nonnull SQLException e, @Nonnull String queryDescription) throws SQLException {
         final var diffMessage = String.format(Locale.ROOT, "‼️ statement failed with the following error at line %s:%n" +
@@ -181,7 +181,8 @@ private static QueryConfig getCheckResultConfig(boolean isExpectedOrdered, @Null
         return new QueryConfig(configName, value, lineNumber, executionContext) {
 
             @Override
-            void checkResultInternal(@Nonnull String currentQuery, @Nonnull Object actual, @Nonnull String queryDescription) throws SQLException {
+            void checkResultInternal(@Nonnull String currentQuery, @Nonnull Object actual,
+                                     @Nonnull String queryDescription, @Nonnull List<String> setups) throws SQLException {
                 logger.debug("⛳️ Matching results of query '{}'", queryDescription);
                 try (RelationalResultSet resultSet = (RelationalResultSet)actual) {
                     final var matchResult = Matchers.matchResultSet(getVal(), resultSet, isExpectedOrdered);
@@ -220,7 +221,8 @@ String decorateQuery(@Nonnull String query) {
 
             @SuppressWarnings({"PMD.CloseResource", "PMD.EmptyWhileStmt"}) // lifetime of autocloseable resource persists beyond method
             @Override
-            void checkResultInternal(@Nonnull String currentQuery, @Nonnull Object actual, @Nonnull String queryDescription) throws SQLException {
+            void checkResultInternal(@Nonnull String currentQuery, @Nonnull Object actual,
+                                     @Nonnull String queryDescription, @Nonnull List<String> setups) throws SQLException {
                 logger.debug("⛳️ Matching plan for query '{}'", queryDescription);
                 final var resultSet = (RelationalResultSet) actual;
                 resultSet.next();
@@ -231,6 +233,7 @@ void checkResultInternal(@Nonnull String currentQuery, @Nonnull Object actual, @
                 final var identifier = PlannerMetricsProto.Identifier.newBuilder()
                         .setBlockName(blockName)
                         .setQuery(currentQuery)
+                        .addAllSetups(setups)
                         .build();
                 final var expectedPlannerMetricsInfo = metricsMap.get(identifier);
 
@@ -302,7 +305,7 @@ void checkResultInternal(@Nonnull String currentQuery, @Nonnull Object actual, @
                                             .setInsertReusedCount(actualPlannerMetrics.getLong(8)))
                             .build();
                     if (expectedPlannerMetricsInfo == null) {
-                        executionContext.putMetrics(blockName, currentQuery, lineNumber, actualInfo, true);
+                        executionContext.putMetrics(blockName, currentQuery, lineNumber, actualInfo, setups);
                         executionContext.markDirty();
                         logger.debug("⭐️ Successfully inserted new planner metrics at line {}", getLineNumber());
                     } else {
@@ -331,7 +334,7 @@ void checkResultInternal(@Nonnull String currentQuery, @Nonnull Object actual, @
                                                 actualCountersAndTimers,
                                                 metricsDescriptor.findFieldByName("insert_reused_count"),
                                                 lineNumber);
-                        executionContext.putMetrics(blockName, currentQuery, lineNumber, actualInfo, isDifferent);
+                        executionContext.putMetrics(blockName, currentQuery, lineNumber, actualInfo, setups);
                         if (isDifferent) {
                             if (executionContext.shouldCorrectMetrics()) {
                                 executionContext.markDirty();
@@ -364,7 +367,8 @@ private static QueryConfig getCheckErrorConfig(@Nullable Object value, int lineN
         return new QueryConfig(QUERY_CONFIG_ERROR, value, lineNumber, executionContext) {
 
             @Override
-            void checkResultInternal(@Nonnull String currentQuery, @Nonnull Object actual, @Nonnull String queryDescription) throws SQLException {
+            void checkResultInternal(@Nonnull String currentQuery, @Nonnull Object actual,
+                                     @Nonnull String queryDescription, @Nonnull List<String> setups) throws SQLException {
                 Matchers.ResultSetPrettyPrinter resultSetPrettyPrinter = new Matchers.ResultSetPrettyPrinter();
                 if (actual instanceof ErrorCapturingResultSet) {
                     Matchers.printRemaining((ErrorCapturingResultSet) actual, resultSetPrettyPrinter);
@@ -406,7 +410,8 @@ private static QueryConfig getCheckCountConfig(@Nullable Object value, int lineN
         return new QueryConfig(QUERY_CONFIG_COUNT, value, lineNumber, executionContext) {
 
             @Override
-            void checkResultInternal(@Nonnull String currentQuery, @Nonnull Object actual, @Nonnull String queryDescription) {
+            void checkResultInternal(@Nonnull String currentQuery, @Nonnull Object actual,
+                                     @Nonnull String queryDescription, @Nonnull List<String> setups) {
                 logger.debug("⛳️ Matching count of update query '{}'", queryDescription);
                 if (!Matchers.matches(getVal(), actual)) {
                     reportTestFailure(String.format(Locale.ROOT, "‼️ Expected count value %d, but got %d at line %d",
@@ -428,7 +433,8 @@ String decorateQuery(@Nonnull String query) {
 
             @SuppressWarnings("PMD.CloseResource") // lifetime of autocloseable persists beyond method
             @Override
-            void checkResultInternal(@Nonnull String currentQuery, @Nonnull Object actual, @Nonnull String queryDescription) throws SQLException {
+            void checkResultInternal(@Nonnull String currentQuery, @Nonnull Object actual,
+                                     @Nonnull String queryDescription, @Nonnull List<String> setups) throws SQLException {
                 logger.debug("⛳️ Matching plan hash of query '{}'", queryDescription);
                 final var resultSet = (RelationalResultSet) actual;
                 resultSet.next();
@@ -445,7 +451,8 @@ public static QueryConfig getNoCheckConfig(int lineNumber, @Nonnull YamlExecutio
         return new QueryConfig(QUERY_CONFIG_NO_CHECKS, null, lineNumber, executionContext) {
             @SuppressWarnings("PMD.CloseResource") // lifetime of autocloseable persists beyond method
             @Override
-            void checkResultInternal(@Nonnull String currentQuery, @Nonnull Object actual, @Nonnull String queryDescription) throws SQLException {
+            void checkResultInternal(@Nonnull String currentQuery, @Nonnull Object actual,
+                                     @Nonnull String queryDescription, @Nonnull List<String> setups) throws SQLException {
                 if (actual instanceof RelationalResultSet) {
                     final var resultSet = (RelationalResultSet) actual;
                     // slurp
@@ -462,7 +469,8 @@ void checkResultInternal(@Nonnull String currentQuery, @Nonnull Object actual, @
     private static QueryConfig getMaxRowConfig(@Nonnull Object value, int lineNumber, @Nonnull YamlExecutionContext executionContext) {
         return new QueryConfig(QUERY_CONFIG_MAX_ROWS, value, lineNumber, executionContext) {
             @Override
-            void checkResultInternal(@Nonnull String currentQuery, @Nonnull Object actual, @Nonnull String queryDescription) throws SQLException {
+            void checkResultInternal(@Nonnull String currentQuery, @Nonnull Object actual,
+                                     @Nonnull String queryDescription, @Nonnull List<String> setups) throws SQLException {
                 Assert.failUnchecked("No results to check on a maxRow config");
             }
         };
@@ -472,7 +480,7 @@ private static QueryConfig getSetupConfig(final Object value, final int lineNumb
         return new QueryConfig(QUERY_CONFIG_SETUP, value, lineNumber, executionContext) {
             @Override
             void checkResultInternal(@Nonnull final String currentQuery, @Nonnull final Object actual,
-                                     @Nonnull final String queryDescription) throws SQLException {
+                                     @Nonnull final String queryDescription, @Nonnull List<String> setups) throws SQLException {
                 Assert.failUnchecked("No results to check on a setup config");
             }
         };
@@ -482,7 +490,8 @@ private static QueryConfig getDebuggerConfig(@Nonnull Object value, int lineNumb
         return new QueryConfig(QUERY_CONFIG_DEBUGGER, DebuggerImplementation.valueOf(((String)value).toUpperCase(Locale.ROOT)),
                 lineNumber, executionContext) {
             @Override
-            void checkResultInternal(@Nonnull String currentQuery, @Nonnull Object actual, @Nonnull String queryDescription) throws SQLException {
+            void checkResultInternal(@Nonnull String currentQuery, @Nonnull Object actual,
+                                     @Nonnull String queryDescription, @Nonnull List<String> setups) throws SQLException {
                 Assert.failUnchecked("No results to check on a debugger config");
             }
         };
@@ -496,7 +505,8 @@ public static QueryConfig getSupportedVersionConfig(Object rawVersion, final int
         }
         return new QueryConfig(QUERY_CONFIG_SUPPORTED_VERSION, rawVersion, lineNumber, executionContext) {
             @Override
-            void checkResultInternal(@Nonnull final String currentQuery, @Nonnull final Object actual, @Nonnull final String queryDescription) throws SQLException {
+            void checkResultInternal(@Nonnull final String currentQuery, @Nonnull final Object actual,
+                                     @Nonnull final String queryDescription, @Nonnull final List<String> setups) throws SQLException {
                 // Nothing to do, this query is supported
                 // SupportedVersion configs are not executed
                 Assertions.fail("Supported version configs are not meant to be executed.");
@@ -516,7 +526,8 @@ public static QueryConfig getNoOpConfig(int lineNumber, @Nonnull YamlExecutionCo
         return new QueryConfig(QUERY_CONFIG_NO_OP, null, lineNumber, executionContext) {
             @SuppressWarnings("PMD.CloseResource") // lifetime of autocloseable persists beyond method
             @Override
-            void checkResultInternal(@Nonnull String currentQuery, @Nonnull Object actual, @Nonnull String queryDescription) throws SQLException {
+            void checkResultInternal(@Nonnull String currentQuery, @Nonnull Object actual,
+                                     @Nonnull String queryDescription, @Nonnull List<String> setups) throws SQLException {
                 // This should not be executed
                 Assertions.fail("NoOp Config should not be executed");
             }
@@ -647,7 +658,7 @@ public SkipConfig(final String configMap, final Object value, final int lineNumb
 
         @Override
         void checkResultInternal(@Nonnull final String currentQuery, @Nonnull final Object actual,
-                                 @Nonnull final String queryDescription) throws SQLException {
+                                 @Nonnull final String queryDescription, @Nonnull final List<String> setups) throws SQLException {
             Assertions.fail("Skipped config should not be executed: Line: " + getLineNumber() + " " + message);
         }
 
@@ -673,7 +684,8 @@ public boolean shouldExecute(YamlConnection connection) {
         }
 
         @Override
-        void checkResultInternal(@Nonnull final String currentQuery, @Nonnull final Object actual, @Nonnull final String queryDescription) throws SQLException {
+        void checkResultInternal(@Nonnull final String currentQuery, @Nonnull final Object actual,
+                                 @Nonnull final String queryDescription, @Nonnull final List<String> setups) throws SQLException {
             Assertions.fail("Check version config should not be executed: Line: " + getLineNumber());
         }