walterddr commented on code in PR #9806:
URL: https://github.com/apache/pinot/pull/9806#discussion_r1024170893
##########
pinot-query-runtime/src/test/java/org/apache/pinot/query/runtime/QueryRunnerTestBase.java:
##########
@@ -292,6 +314,8 @@ public static class Query {
public String _description;
@JsonProperty("outputs")
public List<List<Object>> _outputs = Collections.emptyList();
+ @JsonProperty("expect")
+ public String _expect;
Review Comment:
nit: `expected_exception`?
##########
pinot-query-runtime/src/test/java/org/apache/pinot/query/runtime/QueryRunnerTestBase.java:
##########
@@ -131,6 +137,12 @@ protected void compareRowEquals(List<Object[]> resultRows,
List<Object[]> expect
return ((String) l).compareTo((String) r);
} else if (l instanceof Boolean) {
return ((Boolean) l).compareTo((Boolean) r);
+ } else if (l instanceof BigDecimal) {
+ if (r instanceof BigDecimal) {
+ return ((BigDecimal) l).compareTo((BigDecimal) r);
+ } else {
+ return ((BigDecimal) l).compareTo(new BigDecimal((String) r));
Review Comment:
is this b/c the return type of Pinot is not BigDecimal or b/c H2 is not
returning BigDecimal?
##########
pinot-query-runtime/src/test/java/org/apache/pinot/query/runtime/QueryRunnerTestBase.java:
##########
@@ -151,6 +163,10 @@ protected void compareRowEquals(List<Object[]> resultRows,
List<Object[]> expect
Object[] resultRow = resultRows.get(i);
Object[] expectedRow = expectedRows.get(i);
for (int j = 0; j < resultRow.length; j++) {
+ if (j >= expectedRow.length) {
+ throw new AssertException(String.format("Unexpected row size
mismatch. Expected: %s, Actual: %s",
+ Arrays.toString(expectedRow), Arrays.toString(resultRow)));
+ }
Review Comment:
good catch!!!
##########
pinot-query-runtime/src/test/java/org/apache/pinot/query/runtime/queries/ResourceBasedQueriesTest.java:
##########
@@ -153,20 +158,56 @@ public void tearDown() {
// TODO: name the test using testCaseName for testng reports
@Test(dataProvider = "testResourceQueryTestCaseProviderInputOnly")
- public void testQueryTestCasesWithH2(String testCaseName, String sql)
+ public void testQueryTestCasesWithH2(String testCaseName, String sql, String
expect)
throws Exception {
// query pinot
- List<Object[]> resultRows = queryRunner(sql);
+ Optional<List<Object[]>> resultRows = runQuery(sql, expect);
+ if (!resultRows.isPresent()) {
+ // successfully caught error
+ return;
+ }
+
// query H2 for data
List<Object[]> expectedRows = queryH2(sql);
- compareRowEquals(resultRows, expectedRows);
+ compareRowEquals(resultRows.get(), expectedRows);
}
@Test(dataProvider = "testResourceQueryTestCaseProviderBoth")
- public void testQueryTestCasesWithOutput(String testCaseName, String sql,
List<Object[]> expectedRows)
+ public void testQueryTestCasesWithOutput(String testCaseName, String sql,
List<Object[]> expectedRows, String expect)
throws Exception {
- List<Object[]> resultRows = queryRunner(sql);
- compareRowEquals(resultRows, expectedRows);
+ Optional<List<Object[]>> resultRows = runQuery(sql, expect);
Review Comment:
this refactoring seems convoluted. can't we simply do:
```suggestion
List<Object[]> resultRows;
if (except != null) {
try {
resultRows = queryRunner(SQL);
Assert.fail("Expected error with message '" + except + "'. But
instead rows were returned: " + ...);
} catch (Exception e) {
Assert.assertTrue(...PatternMatch)
}
} else {
resultRows = queryRunner(sql);
}
```
IIRC Assert.fail throws AssertionError which is not an Exception so it won't
be caught in the catch Exception clause.
--
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: commits-unsubscr...@pinot.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
