gortiz commented on code in PR #13255:
URL: https://github.com/apache/pinot/pull/13255#discussion_r1618580395
##########
pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/NullHandlingIntegrationTest.java:
##########
@@ -219,97 +213,187 @@ public void
testTotalCountWithNullHandlingQueryOptionEnabled(boolean useMultiSta
public void testNullLiteralSelectionOnlyBroker(boolean
useMultiStageQueryEngine)
throws Exception {
setUseMultiStageQueryEngine(useMultiStageQueryEngine);
+ // V2 handles such queries in the servers where all rows in the table are
matched and hence the number of rows
+ // returned in the result set is equal to the total number of rows in the
table (instead of a single row like in
+ // V1 where it is handled in the broker). The V2 way is more standard
though and matches behavior in other
+ // databases like Postgres.
notSupportedInV2();
+
// Null literal only
String sqlQuery = "SELECT null FROM mytable
OPTION(enableNullHandling=true)";
JsonNode response = postQuery(sqlQuery);
JsonNode rows = response.get("resultTable").get("rows");
assertTrue(response.get("exceptions").isEmpty());
- assertEquals(rows.size(), 1);
- assertEquals(rows.get(0).get(0).asText(), "null");
+ assertEquals(1, rows.size());
+ assertEquals("null", rows.get(0).get(0).asText());
// Null related functions
sqlQuery = "SELECT isNull(null) FROM " + getTableName() + " OPTION
(enableNullHandling=true);";
response = postQuery(sqlQuery);
rows = response.get("resultTable").get("rows");
assertTrue(response.get("exceptions").isEmpty());
- assertEquals(rows.size(), 1);
- assertEquals(rows.get(0).get(0).asBoolean(), true);
+ assertEquals(1, rows.size());
+ assertTrue(rows.get(0).get(0).asBoolean());
sqlQuery = "SELECT isNotNull(null) FROM " + getTableName() + " OPTION
(enableNullHandling=true);";
response = postQuery(sqlQuery);
rows = response.get("resultTable").get("rows");
assertTrue(response.get("exceptions").isEmpty());
- assertEquals(rows.size(), 1);
- assertEquals(rows.get(0).get(0).asBoolean(), false);
+ assertEquals(1, rows.size());
+ assertFalse(rows.get(0).get(0).asBoolean());
sqlQuery = "SELECT coalesce(null, 1) FROM " + getTableName() + " OPTION
(enableNullHandling=true);";
response = postQuery(sqlQuery);
rows = response.get("resultTable").get("rows");
assertTrue(response.get("exceptions").isEmpty());
- assertEquals(rows.size(), 1);
- assertEquals(rows.get(0).get(0).asInt(), 1);
+ assertEquals(1, rows.size());
+ assertEquals(1, rows.get(0).get(0).asInt());
sqlQuery = "SELECT coalesce(null, null) FROM " + getTableName() + "
OPTION (enableNullHandling=true);";
response = postQuery(sqlQuery);
rows = response.get("resultTable").get("rows");
assertTrue(response.get("exceptions").isEmpty());
- assertEquals(rows.size(), 1);
- assertEquals(rows.get(0).get(0).asText(), "null");
+ assertEquals(1, rows.size());
+ assertEquals("null", rows.get(0).get(0).asText());
sqlQuery = "SELECT isDistinctFrom(null, null) FROM " + getTableName() + "
OPTION (enableNullHandling=true);";
response = postQuery(sqlQuery);
rows = response.get("resultTable").get("rows");
assertTrue(response.get("exceptions").isEmpty());
- assertEquals(rows.size(), 1);
- assertEquals(rows.get(0).get(0).asBoolean(), false);
+ assertEquals(1, rows.size());
+ assertFalse(rows.get(0).get(0).asBoolean());
sqlQuery = "SELECT isNotDistinctFrom(null, null) FROM " + getTableName() +
" OPTION (enableNullHandling=true);";
response = postQuery(sqlQuery);
rows = response.get("resultTable").get("rows");
assertTrue(response.get("exceptions").isEmpty());
- assertEquals(rows.size(), 1);
- assertEquals(rows.get(0).get(0).asBoolean(), true);
+ assertEquals(1, rows.size());
+ assertTrue(rows.get(0).get(0).asBoolean());
sqlQuery = "SELECT isDistinctFrom(null, 1) FROM " + getTableName() + "
OPTION (enableNullHandling=true);";
response = postQuery(sqlQuery);
rows = response.get("resultTable").get("rows");
assertTrue(response.get("exceptions").isEmpty());
- assertEquals(rows.size(), 1);
- assertEquals(rows.get(0).get(0).asBoolean(), true);
+ assertEquals(1, rows.size());
+ assertTrue(rows.get(0).get(0).asBoolean());
sqlQuery = "SELECT isNotDistinctFrom(null, 1) FROM " + getTableName() + "
OPTION (enableNullHandling=true);";
response = postQuery(sqlQuery);
rows = response.get("resultTable").get("rows");
assertTrue(response.get("exceptions").isEmpty());
- assertEquals(rows.size(), 1);
- assertEquals(rows.get(0).get(0).asBoolean(), false);
+ assertEquals(1, rows.size());
+ assertFalse(rows.get(0).get(0).asBoolean());
sqlQuery = "SELECT case when true then null end FROM " + getTableName() +
" OPTION (enableNullHandling=true);";
response = postQuery(sqlQuery);
rows = response.get("resultTable").get("rows");
assertTrue(response.get("exceptions").isEmpty());
- assertEquals(rows.size(), 1);
- assertEquals(rows.get(0).get(0).asText(), "null");
+ assertEquals(1, rows.size());
+ assertEquals("null", rows.get(0).get(0).asText());
sqlQuery = "SELECT case when false then 1 end FROM " + getTableName() + "
OPTION (enableNullHandling=true);";
response = postQuery(sqlQuery);
rows = response.get("resultTable").get("rows");
assertTrue(response.get("exceptions").isEmpty());
- assertEquals(rows.size(), 1);
- assertEquals(rows.get(0).get(0).asText(), "null");
+ assertEquals(1, rows.size());
+ assertEquals("null", rows.get(0).get(0).asText());
// Null intolerant functions
sqlQuery = "SELECT add(null, 1) FROM " + getTableName() + " OPTION
(enableNullHandling=true);";
response = postQuery(sqlQuery);
rows = response.get("resultTable").get("rows");
assertTrue(response.get("exceptions").isEmpty());
- assertEquals(rows.size(), 1);
- assertEquals(rows.get(0).get(0).asText(), "null");
+ assertEquals(1, rows.size());
+ assertEquals("null", rows.get(0).get(0).asText());
+ }
+
+ @Test
+ public void testNullLiteralSelectionInV2() throws Exception {
Review Comment:
Can we rewrite these tests as one method per test or, maybe better, one
provider with the query and the expected results?
We have tons of tests like this (or the one above) where the test has a lot
of assertions. That is an anti-pattern we should try to get rid of. We never
have time to modify the old code, but we should try to write new tests in a
better way.
Just to be clear, there are two main advantages of the suggested approach:
1. The tests are easier to read (each test is a query and an expected result)
2. In case some test fail, the following tests are executed. In the current
version if test 2 fails, 3, 4, etc are not executed, which means it can be more
difficult to find the actual problem (or to know if there are more than one!)
--
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
