Fix non-sqlcompat validation in CalciteWindowQueryTest

processing/src/main/java/org/apache/druid/common/config/NullHandling.java

@@ -22,6 +22,7 @@
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Strings;
 import com.google.inject.Inject;
+import org.apache.druid.segment.column.ValueType;
 import org.apache.druid.segment.data.Indexed;
 
 import javax.annotation.Nullable;
@@ -161,6 +162,28 @@ public static <T> T defaultValueForClass(final Class<T> clazz)
     }
   }
 
+  /**
+   * Returns the default value for the given {@link ValueType}.
+   *
+   * May be null or non-null based on the current SQL-compatible null handling mode.
+   */
+  @Nullable
+  @SuppressWarnings("unchecked")
+  public static Object defaultValueForType(ValueType type)
+  {
+    if (type == ValueType.FLOAT) {
+      return defaultFloatValue();
+    } else if (type == ValueType.DOUBLE) {
+      return defaultDoubleValue();
+    } else if (type == ValueType.LONG) {
+      return defaultLongValue();
+    } else if (type == ValueType.STRING) {
+      return defaultStringValue();
+    } else {
+      return null;
+    }
+  }
+
   public static boolean isNullOrEquivalent(@Nullable String value)
   {
     return replaceWithDefault() ? Strings.isNullOrEmpty(value) : value == null;

sql/src/test/java/org/apache/druid/sql/calcite/BaseCalciteQueryTest.java

@@ -86,6 +86,7 @@
 import org.apache.druid.server.security.ForbiddenException;
 import org.apache.druid.server.security.ResourceAction;
 import org.apache.druid.sql.SqlStatementFactory;
+import org.apache.druid.sql.calcite.QueryTestRunner.QueryResults;
 import org.apache.druid.sql.calcite.expression.DruidExpression;
 import org.apache.druid.sql.calcite.planner.Calcites;
 import org.apache.druid.sql.calcite.planner.PlannerConfig;
@@ -120,18 +121,22 @@
 import org.junit.rules.TemporaryFolder;
 
 import javax.annotation.Nullable;
+
 import java.io.IOException;
 import java.io.PrintStream;
 import java.util.Arrays;
 import java.util.Collection;
 import java.util.HashMap;
 import java.util.List;
+import java.util.Locale;
 import java.util.Map;
 import java.util.Properties;
 import java.util.Set;
 import java.util.function.Consumer;
 import java.util.stream.Collectors;
 
+import static org.junit.Assert.assertEquals;
+
 /**
  * A base class for SQL query testing. It sets up query execution environment, provides useful helper methods,
  * and populates data using {@link CalciteTests#createMockWalker}.
@@ -1054,6 +1059,39 @@
     }
   }
 
+  /**
+   * Validates the results with slight loosening in case {@link NullHandling} is not sql compatible.
+   *
+   * In case {@link NullHandling#replaceWithDefault()} an expected results of <code>null</code> accepts
+   * both <code>null</code> and the default value for that column as actual result.
+   */
+  public void assertResultsValid(String message, List<Object[]> expected, QueryResults queryResults)
+  {
+    List<Object[]> results = queryResults.results;
+    int numRows = Math.min(results.size(), expected.size());
+    for (int row = 0; row < numRows; row++) {
+      Object[] expectedRow = expected.get(row);
+      Object[] resultRow = results.get(row);
+      assertEquals("column count mismatch; at row#" + row, expectedRow.length, resultRow.length);
+
+      for (int i = 0; i < resultRow.length; i++) {
+        Object resultCell = resultRow[i];
+        Object expectedCell = expectedRow[i];
+
+        if (expectedCell == null) {
+          if (resultCell == null) {
+            continue;
+          }
+          expectedCell = NullHandling.defaultValueForType(queryResults.signature.getColumnType(i).get().getType());
+        }
+        assertEquals(
+            String.format(Locale.ENGLISH, "column content mismatch at %d,%d", row, i),
+            expectedCell,
+            resultCell);
+      }
+    }
+  }
+
   public void assertResultsEquals(String sql, List<Object[]> expectedResults, List<Object[]> results)
   {
     for (int i = 0; i < results.size(); i++) {

sql/src/test/java/org/apache/druid/sql/calcite/CalciteWindowQueryTest.java

@@ -20,7 +20,6 @@
 package org.apache.druid.sql.calcite;
 
 import com.fasterxml.jackson.annotation.JsonProperty;
-import com.fasterxml.jackson.core.JsonProcessingException;
 import com.fasterxml.jackson.databind.ObjectMapper;
 import com.fasterxml.jackson.dataformat.yaml.YAMLFactory;
 import com.google.common.collect.ImmutableMap;
@@ -35,6 +34,8 @@
 import org.apache.druid.segment.column.ColumnType;
 import org.apache.druid.segment.column.RowSignature;
 import org.apache.druid.sql.calcite.CalciteWindowQueryTest.WindowQueryTestInputClass.TestType;
+import org.apache.druid.sql.calcite.QueryTestRunner.QueryResults;
+import org.apache.druid.sql.calcite.QueryVerification.QueryResultsVerifier;
 import org.apache.druid.sql.calcite.planner.PlannerContext;
 import org.hamcrest.Matchers;
 import org.junit.Assert;
@@ -43,18 +44,16 @@
 import org.junit.runners.Parameterized;
 
 import java.io.File;
-import java.io.IOException;
 import java.net.URL;
 import java.util.Arrays;
 import java.util.List;
 import java.util.Locale;
 import java.util.Objects;
-import java.util.function.Function;
 
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
 import static org.junit.Assume.assumeThat;
-import static org.junit.Assume.assumeTrue;
 
 /**
  * These tests are file-based, look in resources -> calcite/tests/window for the set of test specifications.
@@ -79,9 +78,7 @@ public static Object parametersForWindowQueryTest() throws Exception
     final URL windowFolderUrl = ClassLoader.getSystemResource("calcite/tests/window");
     File windowFolder = new File(windowFolderUrl.toURI());
 
-    final File[] listedFiles = windowFolder.listFiles(
-        pathname -> pathname.getName().toLowerCase(Locale.ROOT).endsWith(".sqltest")
-    );
+    final File[] listedFiles = windowFolder.listFiles(pathname -> pathname.getName().toLowerCase(Locale.ROOT).endsWith(".sqltest"));
 
     return Arrays
         .stream(Objects.requireNonNull(listedFiles))
@@ -91,119 +88,118 @@ public static Object parametersForWindowQueryTest() throws Exception
 
   private final String filename;
 
-  public CalciteWindowQueryTest(
-      String filename
-  )
+  public CalciteWindowQueryTest(String filename) throws Exception
   {
+
     this.filename = filename;
   }
 
-  @Test
-  @SuppressWarnings("unchecked")
-  public void windowQueryTest() throws IOException
+  class TestCase implements QueryResultsVerifier
   {
-    assumeTrue("These tests are only run in sqlCompatible mode!", NullHandling.sqlCompatible());
-    final Function<String, String> stringManipulator;
-    if (NullHandling.sqlCompatible()) {
-      stringManipulator = s -> "".equals(s) ? null : s;
-    } else {
-      stringManipulator = Function.identity();
+    private WindowQueryTestInputClass input;
+    private ObjectMapper queryJackson;
+
+    public TestCase(String filename) throws Exception
+    {
+      final URL systemResource = ClassLoader.getSystemResource("calcite/tests/window/" + filename);
+
+      final Object objectFromYaml = YAML_JACKSON.readValue(systemResource, Object.class);
+
+      queryJackson = queryFramework().queryJsonMapper();
+      input = queryJackson.convertValue(objectFromYaml, WindowQueryTestInputClass.class);
+
     }
 
-    final URL systemResource = ClassLoader.getSystemResource("calcite/tests/window/" + filename);
+    public TestType getType()
+    {
+      return input.type;
+    }
 
-    final Object objectFromYaml = YAML_JACKSON.readValue(systemResource, Object.class);
+    public String getSql()
+    {
+      return input.sql;
+    }
 
-    final ObjectMapper queryJackson = queryFramework().queryJsonMapper();
-    final WindowQueryTestInputClass input = queryJackson.convertValue(objectFromYaml, WindowQueryTestInputClass.class);
+    @Override
+    public void verifyResults(QueryResults results) throws Exception
+    {
+      if (results.exception != null) {
+        throw new RE(results.exception, "Failed to execute because of exception.");
+      }
+      Assert.assertEquals(1, results.recordedQueries.size());
+
+      final WindowOperatorQuery query = getWindowOperatorQuery(results.recordedQueries);
+      for (int i = 0; i < input.expectedOperators.size(); ++i) {
+        final OperatorFactory expectedOperator = input.expectedOperators.get(i);
+        final OperatorFactory actualOperator = query.getOperators().get(i);
+        if (!expectedOperator.validateEquivalent(actualOperator)) {
+          assertEquals("Operator Mismatch, index[" + i + "]",
+              queryJackson.writeValueAsString(expectedOperator),
+              queryJackson.writeValueAsString(actualOperator));
+          fail("validateEquivalent failed; but textual comparision of operators didn't reported the mismatch!");
+        }
+      }
+      final RowSignature outputSignature = query.getRowSignature();
+      ColumnType[] types = new ColumnType[outputSignature.size()];
+      for (int i = 0; i < outputSignature.size(); ++i) {
+        types[i] = outputSignature.getColumnType(i).get();
+        Assert.assertEquals(types[i], results.signature.getColumnType(i).get());
+      }
 
-    Function<Object, String> jacksonToString = value -> {
-      try {
-        return queryJackson.writeValueAsString(value);
+      maybeDumpActualResults(results.results);
+      for (Object[] result : input.expectedResults) {
+        for (int i = 0; i < result.length; i++) {
+          // Jackson deserializes numbers as the minimum size required to
+          // store the value. This means that
+          // Longs can become Integer objects and then they fail equality
+          // checks. We read the expected
+          // results using Jackson, so, we coerce the expected results to the
+          // type expected.
+          if (result[i] != null) {
+            if (result[i] instanceof Number) {
+              switch (types[i].getType()) {
+                case LONG:
+                  result[i] = ((Number) result[i]).longValue();
+                  break;
+                case DOUBLE:
+                  result[i] = ((Number) result[i]).doubleValue();
+                  break;
+                case FLOAT:
+                  result[i] = ((Number) result[i]).floatValue();
+                  break;
+                default:
+                  throw new ISE("result[%s] was type[%s]!?  Expected it to be numerical", i, types[i].getType());
+              }
+            }
+          }
+        }
       }
-      catch (JsonProcessingException e) {
-        throw new RE(e);
+      assertResultsValid(filename, input.expectedResults, results);
+    }
+
+    private void maybeDumpActualResults(List<Object[]> results) throws Exception
+    {
+      for (Object[] row : results) {
+        System.out.println("  - " + queryJackson.writeValueAsString(row));
       }
-    };
+    }
+  }
+
+  @Test
+  @SuppressWarnings("unchecked")
+  public void windowQueryTest() throws Exception
+  {
+    TestCase testCase = new TestCase(filename);
 
-    assumeThat(input.type, Matchers.not(TestType.failingTest));
+    assumeThat(testCase.getType(), Matchers.not(TestType.failingTest));
 
-    if (input.type == TestType.operatorValidation) {
+    if (testCase.getType() == TestType.operatorValidation) {
       testBuilder()
           .skipVectorize(true)
-          .sql(input.sql)
+          .sql(testCase.getSql())
           .queryContext(ImmutableMap.of(PlannerContext.CTX_ENABLE_WINDOW_FNS, true,
               QueryContexts.ENABLE_DEBUG, true))
-          .addCustomVerification(QueryVerification.ofResults(results -> {
-            if (results.exception != null) {
-              throw new RE(results.exception, "Failed to execute because of exception.");
-            }
-
-            Assert.assertEquals(1, results.recordedQueries.size());
-            // 2 tests are failing at this moment on this check
-            // They are wikipediaFramedAggregations.sqlTest and wikipediaAggregationsMultipleOrdering.sqlTest
-            // Calcite 1.35 plans them as an external scan over a windowOperator
-            // with an additional COUNT(*) to replace intervals with no data
-            // and then adding a virtual column to filter it out
-            // For example, ExpressionVirtualColumn{name='v0', expression='case_searched(("w0" > 0),"w1",null
-            // and aggregations=[CountAggregatorFactory{name='w0'}, LongSumAggregatorFactory{fieldName='a0', expression='null', name='w1'}]}}]}
-            // These 2 tests are marked as failingTests to unblock testing at this moment
-
-            final WindowOperatorQuery query = getWindowOperatorQuery(results.recordedQueries);
-            for (int i = 0; i < input.expectedOperators.size(); ++i) {
-              final OperatorFactory expectedOperator = input.expectedOperators.get(i);
-              final OperatorFactory actualOperator = query.getOperators().get(i);
-              if (!expectedOperator.validateEquivalent(actualOperator)) {
-                // This assertion always fails because the validate equivalent failed, but we do it anyway
-                // so that we get values in the output of the failed test to make it easier to
-                // debug what happened.  Note, we use the Jackson representation when showing the diff.  There is
-                // a chance that this representation is exactly equivalent, but the validation call is still failing
-                // this is probably indicative of a bug where something that needs to be serialized by Jackson
-                // currently is not.  Check your getters.
-
-                // prepend different values so that we are guaranteed that it is always different
-                String expected = "e " + jacksonToString.apply(expectedOperator);
-                String actual = "a " + jacksonToString.apply(actualOperator);
-
-                Assert.assertEquals("Operator Mismatch, index[" + i + "]", expected, actual);
-              }
-            }
-            final RowSignature outputSignature = query.getRowSignature();
-            ColumnType[] types = new ColumnType[outputSignature.size()];
-            for (int i = 0; i < outputSignature.size(); ++i) {
-              types[i] = outputSignature.getColumnType(i).get();
-              Assert.assertEquals(types[i], results.signature.getColumnType(i).get());
-            }
-
-            maybeDumpActualResults(jacksonToString, results.results);
-            for (Object[] result : input.expectedResults) {
-              for (int i = 0; i < result.length; i++) {
-                // Jackson deserializes numbers as the minimum size required to store the value.  This means that
-                // Longs can become Integer objects and then they fail equality checks.  We read the expected
-                // results using Jackson, so, we coerce the expected results to the type expected.
-                if (result[i] != null) {
-                  if (result[i] instanceof Number) {
-                    switch (types[i].getType()) {
-                      case LONG:
-                        result[i] = ((Number) result[i]).longValue();
-                        break;
-                      case DOUBLE:
-                        result[i] = ((Number) result[i]).doubleValue();
-                        break;
-                      case FLOAT:
-                        result[i] = ((Number) result[i]).floatValue();
-                        break;
-                      default:
-                        throw new ISE("result[%s] was type[%s]!?  Expected it to be numerical", i, types[i].getType());
-                    }
-                  } else if (result[i] instanceof String) {
-                    result[i] = stringManipulator.apply((String) result[i]);
-                  }
-                }
-              }
-            }
-            assertResultsEquals(filename, input.expectedResults, results.results);
-          }))
+          .addCustomVerification(QueryVerification.ofResults(testCase))
           .run();
     }
   }
@@ -216,16 +212,6 @@ private WindowOperatorQuery getWindowOperatorQuery(List<Query<?>> queries)
     return (WindowOperatorQuery) query;
   }
 
-  private void maybeDumpActualResults(
-      Function<Object, String> toStrFn, List<Object[]> results
-  )
-  {
-    if (DUMP_ACTUAL_RESULTS) {
-      for (Object[] result : results) {
-        System.out.println("  - " + toStrFn.apply(result));
-      }
-    }
-  }
 
   public static class WindowQueryTestInputClass
   {