rohangarg commented on code in PR #15086:
URL: https://github.com/apache/druid/pull/15086#discussion_r1348795205
##########
sql/src/test/java/org/apache/druid/sql/calcite/CalciteWindowQueryTest.java:
##########
@@ -91,119 +88,117 @@ public static Object parametersForWindowQueryTest()
throws Exception
private final String filename;
- public CalciteWindowQueryTest(
- String filename
- )
+ public CalciteWindowQueryTest(String filename)
{
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
Review Comment:
this comment seems to have been dropped in the refactor - is that intended?
I still see that those tests are in `operatorValidation` type.
##########
sql/src/test/java/org/apache/druid/sql/calcite/BaseCalciteQueryTest.java:
##########
@@ -1054,6 +1059,39 @@ public Map<String, Object> baseQueryContext()
}
}
+ /**
+ * 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());
Review Comment:
is there a reason why we aren't checking the size to be equal? Also, maybe
the name of the assertion method can be better to indicate that it relaxes the
default value case to also accept nulls
##########
sql/src/test/java/org/apache/druid/sql/calcite/CalciteWindowQueryTest.java:
##########
@@ -91,119 +88,117 @@ public static Object parametersForWindowQueryTest()
throws Exception
private final String filename;
- public CalciteWindowQueryTest(
- String filename
- )
+ public CalciteWindowQueryTest(String filename)
{
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));
Review Comment:
since we're rewriting this code - maybe we can write this through a logger
##########
sql/src/test/java/org/apache/druid/sql/calcite/BaseCalciteQueryTest.java:
##########
@@ -1054,6 +1059,39 @@ public Map<String, Object> baseQueryContext()
}
}
+ /**
+ * 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
Review Comment:
Do you mean to say `In case {@link NullHandling#replaceWithDefault()} is
true, an expected...` ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
