diff --git a/query/ir/exec/NLEntityProperties.cpp b/query/ir/exec/NLEntityProperties.cpp index 754dccdbb6..c13eec76e0 100644 --- a/query/ir/exec/NLEntityProperties.cpp +++ b/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) diff --git a/query/ir/exec/NLMergeExecutor.cpp b/query/ir/exec/NLMergeExecutor.cpp index 6eb5e77abe..5dc74c2805 100644 --- a/query/ir/exec/NLMergeExecutor.cpp +++ b/query/ir/exec/NLMergeExecutor.cpp @@ -6,10 +6,13 @@ #include #include #include +#include #include #include #include +#include + #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& 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 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& nodes = _data->nodes(); + const std::vector& 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); diff --git a/query/ir/exec/NLWriteProperties.cpp b/query/ir/exec/NLWriteProperties.cpp index 8d0eceafd3..5136859d2c 100644 --- a/query/ir/exec/NLWriteProperties.cpp +++ b/query/ir/exec/NLWriteProperties.cpp @@ -1,6 +1,7 @@ #include "NLWriteProperties.h" #include +#include #include @@ -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, diff --git a/query/ir/exec/NLWriteProperties.h b/query/ir/exec/NLWriteProperties.h index 1fc2928080..593a0ee143 100644 --- a/query/ir/exec/NLWriteProperties.h +++ b/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. diff --git a/test/query/ir/merge/MergeRejectionTest.cpp b/test/query/ir/merge/MergeRejectionTest.cpp index c1f63fc122..f61e2e1538 100644 --- a/test/query/ir/merge/MergeRejectionTest.cpp +++ b/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); }