AD-204 Fixed comparisons against NULL/undefined (#5)

Bruce Irschick · web-flow · commit a73e3c707f00 · 2021-06-10T16:16:57.000-07:00
### Summary AD-204 Fixed comparisons against NULL/undefined ### Description Comparison operators (<, >, !=) now return null in expressions where a field is compared to null. Fixes three types of queries: 1. Queries with multiple conditions on a single field eg. `SELECT * FROM table WHERE field <> 3 AND field <> 5` 2. Queries did not remove null values in not-equals filters. For example in the above query a row containing `field:null` would be included in the result set. 3. Queries involving CASE would incorrectly consider some comparisons to null to be true/false, instead of unknown. eg. `SELECT CASE WHEN field < 2 THEN 'A' ELSE 'B' END FROM table` would result in 'A' if `field==null` instead of 'B'. The same is true if the field does not exist at all in the document. ### Related Issue https://bitquill.atlassian.net/browse/AD-204
diff --git a/calcite-adapter/src/main/java/software/amazon/documentdb/jdbc/calcite/adapter/DocumentDbFilter.java b/calcite-adapter/src/main/java/software/amazon/documentdb/jdbc/calcite/adapter/DocumentDbFilter.java
@@ -152,12 +152,25 @@ private Map<String, Object> translateAnd(final RexNode node0) {
             return map;
         }
 
+        @SuppressWarnings("unchecked")
         private static void addPredicate(final Map<String, Object> map, final String op,
                 final Object v) {
             if (map.containsKey(op) && stronger(op, map.get(op), v)) {
                 return;
             }
-            map.put(op, v);
+            if ("$ne".equals(op)) {
+                if (map.containsKey("$nin") && map.get("$nin") instanceof List) {
+                    final List<Object> vars = (List<Object>) map.get("$nin");
+                    vars.add(v);
+                } else {
+                    final List<Object> vars = new ArrayList<>();
+                    vars.add(null);
+                    vars.add(v);
+                    map.put("$nin", vars);
+                }
+            } else {
+                map.put(op, v);
+            }
         }
 
         /** Returns whether {@code v0} is a stronger value for operator {@code key}
diff --git a/calcite-adapter/src/main/java/software/amazon/documentdb/jdbc/calcite/adapter/DocumentDbRules.java b/calcite-adapter/src/main/java/software/amazon/documentdb/jdbc/calcite/adapter/DocumentDbRules.java
@@ -240,7 +240,16 @@ protected RexToMongoTranslator(final JavaTypeFactory typeFactory,
             }
             final String stdOperator = MONGO_OPERATORS.get(call.getOperator());
             if (stdOperator != null) {
-                return "{" + stdOperator + ": [" + Util.commaList(strings) + "]}";
+                // For comparisons other than equals we must check it exists and is not null.
+                final String op = "{" + stdOperator + ": [" + Util.commaList(strings) + "]}";
+                if (MONGO_OPERATORS.get(SqlStdOperatorTable.LESS_THAN).equals(stdOperator) ||
+                        MONGO_OPERATORS.get(SqlStdOperatorTable.LESS_THAN_OR_EQUAL).equals(stdOperator) ||
+                        MONGO_OPERATORS.get(SqlStdOperatorTable.NOT_EQUALS).equals(stdOperator) ||
+                        MONGO_OPERATORS.get(SqlStdOperatorTable.GREATER_THAN).equals(stdOperator) ||
+                        MONGO_OPERATORS.get(SqlStdOperatorTable.GREATER_THAN_OR_EQUAL).equals(stdOperator)) {
+                    return addNullChecksToQuery(strings, op);
+                }
+                return op;
             }
             if (call.getOperator() == SqlStdOperatorTable.ITEM) {
                 final RexNode op1 = call.operands.get(1);
@@ -283,6 +292,21 @@ protected RexToMongoTranslator(final JavaTypeFactory typeFactory,
                     + " is not supported by MongoProject");
         }
 
+        private String addNullChecksToQuery(final List<String> strings, final String op) {
+            final StringBuilder sb = new StringBuilder("{$and: [");
+            sb.append(op);
+            for (int i = 0; i < 2; i++) {
+                if (!strings.get(i).equals("null")) {
+                    // The operator {$gt null} filters out any values that are null or undefined.
+                    sb.append("{$gt: [");
+                    sb.append(strings.get(i));
+                    sb.append(", null]}");
+                }
+            }
+            sb.append("]}");
+            return sb.toString();
+        }
+
         private static String stripQuotes(final String s) {
             return s.startsWith("'") && s.endsWith("'")
                     ? s.substring(1, s.length() - 1)
diff --git a/calcite-adapter/src/test/java/software/amazon/documentdb/jdbc/query/DocumentDbQueryMappingServiceTest.java b/calcite-adapter/src/test/java/software/amazon/documentdb/jdbc/query/DocumentDbQueryMappingServiceTest.java
@@ -1251,4 +1251,47 @@ void testQueryWithSumOne() throws SQLException {
                         "{\"$group\": {\"_id\": {}, \"EXPR$0\": {\"$sum\": \"$_f0\"}}}"),
                 result.getAggregateOperations().get(1));
     }
+
+    @Test
+    @DisplayName("Tests CASE with one field, and three sections.")
+    void testQueryWithCASE() throws SQLException {
+        final String query =
+                String.format(
+                        "SELECT CASE " +
+                                "WHEN \"field\" > 10 THEN 'A' " +
+                                "WHEN \"field\" > 5 THEN 'B' " +
+                                "ELSE 'C' END FROM \"%s\".\"%s\"", DATABASE_NAME, COLLECTION_NAME + "_array");
+        final DocumentDbMqlQueryContext result = queryMapper.get(query);
+        Assertions.assertNotNull(result);
+        Assertions.assertEquals(COLLECTION_NAME, result.getCollectionName());
+        Assertions.assertEquals(1, result.getColumnMetaData().size());
+        Assertions.assertEquals(3, result.getAggregateOperations().size());
+        Assertions.assertEquals(
+                BsonDocument.parse(
+                        "{\"$match\": " +
+                                "{\"$or\": " +
+                                "[{\"array.field\": {\"$exists\": true}}, {\"array.field1\": {\"$exists\": true}}, " +
+                                "{\"array.field2\": {\"$exists\": true}}]}}"),
+                result.getAggregateOperations().get(0));
+        Assertions.assertEquals(
+                BsonDocument.parse(
+                        "{\"$unwind\": " +
+                                "{\"path\": \"$array\", " +
+                                "\"preserveNullAndEmptyArrays\": true, " +
+                                "\"includeArrayIndex\": \"array_index_lvl_0\"}}"),
+                result.getAggregateOperations().get(1));
+        Assertions.assertEquals(
+                BsonDocument.parse(
+                        "{\"$addFields\": {\"EXPR$0\": " +
+                                "{\"$cond\": [{\"$and\": [" +
+                                "{\"$gt\": [\"$array.field\", {\"$literal\": 10}]}, " +
+                                "{\"$gt\": [\"$array.field\", null]}, " +
+                                "{\"$gt\": [{\"$literal\": 10}, null]}]}, {\"$literal\": \"A\"}, " +
+                                "{\"$cond\": [{\"$and\": [" +
+                                "{\"$gt\": [\"$array.field\", {\"$literal\": 5}]}, " +
+                                "{\"$gt\": [\"$array.field\", null]}, " +
+                                "{\"$gt\": [{\"$literal\": 5}, null]}]}, " +
+                                "{\"$literal\": \"B\"}, {\"$literal\": \"C\"}]}]}}}\n"),
+                result.getAggregateOperations().get(2));
+    }
 }
diff --git a/src/test/java/software/amazon/documentdb/jdbc/DocumentDbStatementTest.java b/src/test/java/software/amazon/documentdb/jdbc/DocumentDbStatementTest.java
@@ -17,6 +17,7 @@
 package software.amazon.documentdb.jdbc;
 
 import org.bson.BsonDocument;
+import org.bson.BsonMinKey;
 import org.junit.jupiter.api.AfterEach;
 import org.junit.jupiter.api.Assertions;
 import org.junit.jupiter.api.BeforeAll;
@@ -1267,7 +1268,7 @@ void testCountColumnName() throws SQLException {
     @Test
     @DisplayName("Tests query with SUM(1).")
     void testQuerySumOne() throws SQLException {
-        final String tableName = "testSumOfNulls";
+        final String tableName = "testQuerySumOne";
         final BsonDocument doc1 = BsonDocument.parse("{\"_id\": 101,\n" +
                 "\"field\": 4}");
         final BsonDocument doc2 = BsonDocument.parse("{\"_id\": 102}");
@@ -1283,6 +1284,148 @@ void testQuerySumOne() throws SQLException {
         Assertions.assertFalse(resultSet.next());
     }
 
+    /**
+     * Tests that queries with not-equals do not return null or undefined values.
+     * @throws SQLException occurs if query fails.
+     */
+    @Test
+    @DisplayName("Tests that comparisons to null do not return a value.")
+    void testComparisonToNull() throws SQLException {
+        final String tableName = "testComparisonsToNull";
+        final BsonDocument doc1 = BsonDocument.parse("{\"_id\": 101,\n" +
+                "\"field\": 4}");
+        final BsonDocument doc2 = BsonDocument.parse("{\"_id\": 102, \n}" +
+                "\"field\": null");
+        final BsonDocument doc3 = BsonDocument.parse("{\"_id\": 103}");
+        insertBsonDocuments(tableName, DATABASE_NAME, USER, PASSWORD,
+                new BsonDocument[]{doc1, doc2, doc3});
+        final Statement statement = getDocumentDbStatement();
+        final ResultSet resultSet = statement.executeQuery(
+                String.format("SELECT * from \"%s\".\"%s\" WHERE \"field\" <> 5", DATABASE_NAME, tableName));
+        Assertions.assertNotNull(resultSet);
+        Assertions.assertTrue(resultSet.next());
+        Assertions.assertFalse(resultSet.next());
+    }
+
+    /**
+     * Tests that queries with multiple not-equals clauses are correct.
+     * @throws SQLException occurs if query fails.
+     */
+    @Test
+    @DisplayName("Tests that multiple != conditions can be used.")
+    void testMultipleNotEquals() throws SQLException {
+        final String tableName = "testMultipleNotEquals";
+        final BsonDocument doc1 = BsonDocument.parse("{\"_id\": 101,\n" +
+                "\"field\": 4}");
+        final BsonDocument doc2 = BsonDocument.parse("{\"_id\": 102, \n" +
+                "\"field\": 3}");
+        final BsonDocument doc3 = BsonDocument.parse("{\"_id\": 103, \n" +
+                "\"field\": 2}");
+        final BsonDocument doc4 = BsonDocument.parse("{\"_id\": 104, \n" +
+                "\"field\": null}");
+        insertBsonDocuments(tableName, DATABASE_NAME, USER, PASSWORD,
+                new BsonDocument[]{doc1, doc2, doc3, doc4});
+        final Statement statement = getDocumentDbStatement();
+        final ResultSet resultSet = statement.executeQuery(
+                String.format("SELECT * from \"%s\".\"%s\" WHERE \"field\" <> 4 AND \"field\" <> 3", DATABASE_NAME, tableName));
+        Assertions.assertNotNull(resultSet);
+        Assertions.assertTrue(resultSet.next());
+        Assertions.assertEquals(2, resultSet.getInt(2));
+        Assertions.assertFalse(resultSet.next());
+    }
+
+    /**
+     * Tests that queries CASE are correct, particularly where null or undefined values are involved.
+     * @throws SQLException occurs if query fails.
+     */
+    @Test
+    @DisplayName("Tests queries with CASE and null values are correct.")
+    void testCASE() throws SQLException {
+        final String tableName = "testCASE";
+        final BsonDocument doc1 = BsonDocument.parse("{\"_id\": 101,\n" +
+                "\"field\": 1}");
+        final BsonDocument doc2 = BsonDocument.parse("{\"_id\": 102,\n" +
+                "\"field\": 2}");
+        final BsonDocument doc3 = BsonDocument.parse("{\"_id\": 103,\n" +
+                "\"field\": 5}");
+        final BsonDocument doc4 = BsonDocument.parse("{\"_id\": 104,\n" +
+                "\"field\": 4}");
+        final BsonDocument doc5 = BsonDocument.parse("{\"_id\": 105,\n" +
+                "\"field\": 3}");
+        final BsonDocument doc6 = BsonDocument.parse("{\"_id\": 106,\n" +
+                "\"field\": null}");
+        final BsonDocument doc7 = BsonDocument.parse("{\"_id\": 107}");
+        final BsonDocument doc8 = BsonDocument.parse("{\"_id\": 108}");
+        doc8.append("field", new BsonMinKey());
+        insertBsonDocuments(tableName, DATABASE_NAME, USER, PASSWORD,
+                new BsonDocument[]{doc1, doc2, doc3, doc4, doc5, doc6, doc7, doc8});
+        final Statement statement = getDocumentDbStatement();
+        final ResultSet resultSet = statement.executeQuery(
+                String.format(
+                        "SELECT CASE " +
+                                "WHEN \"field\" < 2  THEN 'A' " +
+                                "WHEN \"field\" <= 2 THEN 'B' " +
+                                "WHEN \"field\" > 4 THEN 'C' " +
+                                "WHEN \"field\" >= 4 THEN 'D' " +
+                                "WHEN \"field\" <> 7 THEN 'E' " +
+                                "ELSE 'F' END FROM \"%s\".\"%s\"",
+                        DATABASE_NAME, tableName));
+        Assertions.assertNotNull(resultSet);
+        Assertions.assertTrue(resultSet.next());
+        Assertions.assertEquals("A", resultSet.getString(1));
+        Assertions.assertTrue(resultSet.next());
+        Assertions.assertEquals("B", resultSet.getString(1));
+        Assertions.assertTrue(resultSet.next());
+        Assertions.assertEquals("C", resultSet.getString(1));
+        Assertions.assertTrue(resultSet.next());
+        Assertions.assertEquals("D", resultSet.getString(1));
+        Assertions.assertTrue(resultSet.next());
+        Assertions.assertEquals("E", resultSet.getString(1));
+        Assertions.assertTrue(resultSet.next());
+        Assertions.assertEquals("F", resultSet.getString(1));
+        Assertions.assertTrue(resultSet.next());
+        Assertions.assertEquals("F", resultSet.getString(1));
+        Assertions.assertTrue(resultSet.next());
+        Assertions.assertEquals("F", resultSet.getString(1));
+        Assertions.assertFalse(resultSet.next());
+    }
+
+    /**
+     * Tests that queries of CASE are correct with two different fields involved.
+     * @throws SQLException occurs if query fails.
+     */
+    @Test
+    @DisplayName("Tests queries with two field CASE.")
+    void testCASETwoFields() throws SQLException {
+        final String tableName = "testCASETwoFields";
+        final BsonDocument doc1 = BsonDocument.parse("{\"_id\": 101,\n" +
+                "\"fieldA\": 1,\n" +
+                "\"fieldB\": 2}");
+        final BsonDocument doc2 = BsonDocument.parse("{\"_id\": 102,\n" +
+                "\"fieldA\": 2,\n" +
+                "\"fieldB\": 1}");
+        final BsonDocument doc3 = BsonDocument.parse("{\"_id\": 103,\n" +
+                "\"fieldA\": 1}");
+        insertBsonDocuments(tableName, DATABASE_NAME, USER, PASSWORD,
+                new BsonDocument[]{doc1, doc2, doc3});
+        final Statement statement = getDocumentDbStatement();
+        final ResultSet resultSet = statement.executeQuery(
+                String.format(
+                        "SELECT CASE " +
+                                "WHEN \"fieldA\" < \"fieldB\"  THEN 'A' " +
+                                "WHEN \"fieldA\" > \"fieldB\" THEN 'B' " +
+                                "ELSE 'C' END FROM \"%s\".\"%s\"",
+                        DATABASE_NAME, tableName));
+        Assertions.assertNotNull(resultSet);
+        Assertions.assertTrue(resultSet.next());
+        Assertions.assertEquals("A", resultSet.getString(1));
+        Assertions.assertTrue(resultSet.next());
+        Assertions.assertEquals("B", resultSet.getString(1));
+        Assertions.assertTrue(resultSet.next());
+        Assertions.assertEquals("C", resultSet.getString(1));
+        Assertions.assertFalse(resultSet.next());
+    }
+
     protected static DocumentDbStatement getDocumentDbStatement() throws SQLException {
         return getDocumentDbStatement(DocumentDbMetadataScanMethod.RANDOM);
     }
