TUR-313: Query: reject a MERGE key that is null at run time

query/ir/exec/NLEntityProperties.cpp

@@ -122,10 +122,6 @@ MapBuffer<>::MapItemVariant writtenValue(NLWrittenValues& written,
     return PropertyNull {};
 }
 
-bool holdsValue(const CommitWriteBuffer::SupportedTypeVariant& value) {
-    return std::visit([](const auto& held) { return held.has_value(); }, value);
-}
-
 }
 
 NLEntityProperties::NLEntityProperties(NLExecutionContext* context, NLValueStore* readValues)

query/ir/exec/NLMergeExecutor.cpp

@@ -6,10 +6,13 @@
 #include <optional>
 #include <span>
 #include <string>
+#include <string_view>
 #include <unordered_set>
 #include <variant>
 #include <vector>
 
+#include <spdlog/fmt/bundled/format.h>
+
 #include "iterators/GetInEdgesIterator.h"
 #include "iterators/GetOutEdgesIterator.h"
 #include "iterators/GetPropertiesWithNullIterator.h"
@@ -20,6 +23,7 @@
 
 #include "versioning/CommitWriteBuffer.h"
 #include "views/GraphView.h"
+#include "writers/MetadataBuilder.h"
 
 #include "NLExecutionContext.h"
 #include "NLWriteProperties.h"
@@ -146,6 +150,26 @@ void throwIfBoundNodeIsNull(const NLMergeData::Node& node, size_t rowCount) {
     }
 }
 
+// A null matches no stored value, so a merge keyed on one would write the pattern again
+// on every run
+void throwIfKeyIsNull(const MetadataBuilder* metadataBuilder,
+                      const std::vector<NLMergeProperty>& properties,
+                      const NLMergeWorkingSet::PropertiesPerRow& extracted) {
+    for (size_t index = 0; index < properties.size(); index++) {
+        const bool holdsNull = !std::ranges::all_of(extracted[index], [](const CommitWriteBuffer::UntypedProperty& property) {
+            return holdsValue(property.value);
+        });
+
+        if (holdsNull) {
+            const PropertyTypeID propertyTypeID = properties[index]._propertyType._id;
+            const std::optional<std::string_view> name = metadataBuilder->findPropertyTypeName(propertyTypeID);
+            bioassert(name, "Property type {} is not in the change's metadata", propertyTypeID.getValue());
+
+            throw IRException(fmt::format("MERGE cannot match on property '{}', which is null", *name));
+        }
+    }
+}
+
 }
 
 NLMergeExecutor::NLMergeExecutor(NLExecutionContext* context, NLMergeData* data)
@@ -174,14 +198,26 @@ void NLMergeExecutor::run() {
     clearResults();
     extractProperties(rowCount);
 
-    for (const NLMergeData::Node& node : _data->nodes()) {
+    const std::vector<NLMergeData::Node>& nodes = _data->nodes();
+    const std::vector<NLMergeData::Hop>& hops = _data->hops();
+    const MetadataBuilder* metadataBuilder = _context->getMetadataBuilder();
+
+    for (size_t nodeIndex = 0; nodeIndex < nodes.size(); nodeIndex++) {
+        const NLMergeData::Node& node = nodes[nodeIndex];
+
         if (node._boundColumn) {
             throwIfBoundNodeIsNull(node, rowCount);
         }
+
+        throwIfKeyIsNull(metadataBuilder, node._properties, _work->_nodeProperties[nodeIndex]);
+    }
+
+    for (size_t hopIndex = 0; hopIndex < hops.size(); hopIndex++) {
+        throwIfKeyIsNull(metadataBuilder, hops[hopIndex]._properties, _work->_hopProperties[hopIndex]);
     }
 
-    _work->_candidates.resize(_data->nodes().size());
-    _work->_candidateKeys.resize(_data->nodes().size());
+    _work->_candidates.resize(nodes.size());
+    _work->_candidateKeys.resize(nodes.size());
 
     for (size_t row = 0; row < rowCount; row++) {
         matchRow(row);

query/ir/exec/NLWriteProperties.cpp

@@ -1,6 +1,7 @@
 #include "NLWriteProperties.h"
 
 #include <optional>
+#include <variant>
 
 #include <spdlog/fmt/bundled/format.h>
 
@@ -392,6 +393,10 @@ size_t db::committedEdgeCount(const GraphView* view) {
     return reader.getTotalEdgesAllocated();
 }
 
+bool db::holdsValue(const CommitWriteBuffer::SupportedTypeVariant& value) {
+    return std::visit([](const auto& held) { return held.has_value(); }, value);
+}
+
 void db::fillNullProperties(size_t rowCount,
                             PropertyTypeID propID,
                             ValueType valueType,

query/ir/exec/NLWriteProperties.h

@@ -16,6 +16,8 @@ class GraphView;
 size_t committedNodeCount(const GraphView* view);
 size_t committedEdgeCount(const GraphView* view);
 
+bool holdsValue(const CommitWriteBuffer::SupportedTypeVariant& value);
+
 // The values one property column holds, as the write buffer takes them: one entry per
 // row, owning whatever the column only borrows. What a create, a set and a merge all
 // turn a row's asked-for value into before writing it.

test/query/ir/merge/MergeRejectionTest.cpp

@@ -95,6 +95,67 @@ TEST_F(MergeRejectionTest, rejectsAPatternOnANodeAnOptionalMatchMissed) {
         << status.getError();
 }
 
+// A null matches nothing, so a merge keyed on one would write a node on every run. The
+// analyzer turns away a key that is null by type.
+TEST_F(MergeRejectionTest, rejectsANodeKeyThatIsNullByType) {
+    QueryStatus status;
+    runQuery("MERGE (t:Tag {name: 'k', flag: 1 < 'a'})", status);
+
+    EXPECT_FALSE(status.isOk()) << status.getError();
+    EXPECT_NE(status.getError().find("MERGE cannot match on property 'flag', which is null"), std::string::npos)
+        << status.getError();
+}
+
+TEST_F(MergeRejectionTest, rejectsANodeKeyThatIsNullOnOneRow) {
+    QueryStatus status;
+    runQuery("UNWIND [null, 'a'] AS v MERGE (t:Tag {name: v}) RETURN t.name", status);
+
+    EXPECT_FALSE(status.isOk()) << status.getError();
+    EXPECT_NE(status.getError().find("MERGE cannot match on property 'name', which is null"), std::string::npos)
+        << status.getError();
+}
+
+TEST_F(MergeRejectionTest, rejectsANodeKeyThatIsNullOnTheLastRow) {
+    QueryStatus status;
+    runQuery("UNWIND ['a', 'b', null] AS v MERGE (t:Tag {name: v})", status);
+
+    EXPECT_FALSE(status.isOk()) << status.getError();
+    EXPECT_NE(status.getError().find("MERGE cannot match on property 'name', which is null"), std::string::npos)
+        << status.getError();
+}
+
+TEST_F(MergeRejectionTest, rejectsANodeKeyACaseMakesNullOnOneRow) {
+    QueryStatus status;
+    runQuery("MATCH (p:Person) "
+             "MERGE (t:Tag {name: CASE WHEN p.name = 'Remy' THEN null ELSE p.name END})",
+             status);
+
+    EXPECT_FALSE(status.isOk()) << status.getError();
+    EXPECT_NE(status.getError().find("MERGE cannot match on property 'name', which is null"), std::string::npos)
+        << status.getError();
+}
+
+// Remy and Adam have an age, the other Person nodes do not
+TEST_F(MergeRejectionTest, rejectsANodeKeyReadFromAMissingProperty) {
+    QueryStatus status;
+    runQuery("MATCH (p:Person) MERGE (t:Tag {name: 'x', age: p.age})", status);
+
+    EXPECT_FALSE(status.isOk()) << status.getError();
+    EXPECT_NE(status.getError().find("MERGE cannot match on property 'age', which is null"), std::string::npos)
+        << status.getError();
+}
+
+TEST_F(MergeRejectionTest, rejectsAnEdgeKeyThatIsNullOnOneRow) {
+    QueryStatus status;
+    runQuery("MATCH (a:Person {name: 'Remy'}), (b:Person {name: 'Adam'}) "
+             "UNWIND [1, null] AS w MERGE (a)-[:PROBE {weight: w}]->(b)",
+             status);
+
+    EXPECT_FALSE(status.isOk()) << status.getError();
+    EXPECT_NE(status.getError().find("MERGE cannot match on property 'weight', which is null"), std::string::npos)
+        << status.getError();
+}
+
 int main(int argc, char** argv) {
     return turing::test::turingTestMain(argc, argv);
 }