Extend node property mapping validation to default values

Author: s1ck

doc/modules/ROOT/pages/machine-learning/linkprediction-pipelines/training.adoc

@@ -312,7 +312,7 @@ CALL gds.graph.project(
   'fullGraph',
   {
     Person: {
-      properties: ['age']
+      properties: {age: {defaultValue: 1}}
     },
     City: {
       properties: {age: {defaultValue: 1}}

graph-projection-api/src/main/java/org/neo4j/gds/AbstractNodeProjections.java

@@ -27,6 +27,7 @@
 import java.util.HashMap;
 import java.util.LinkedHashMap;
 import java.util.Map;
+import java.util.Objects;
 
 import static java.util.Collections.emptyMap;
 import static java.util.Collections.singletonMap;
@@ -175,23 +176,39 @@ private static void validateIdentifierName(String identifier) {
 
     @Value.Check
     public void validatePropertyKeyMappings() {
-        var mapping = new HashMap<String, String>();
+        var seenMappings = new HashMap<String, PropertyMapping>();
 
         projections().values().stream()
             .flatMap(nodeProjection -> nodeProjection.properties().stream())
             .forEach(propertyMapping -> {
                 var propertyKey = propertyMapping.propertyKey();
-                var neoKey = propertyMapping.neoPropertyKey();
-
-                if (mapping.containsKey(propertyKey) && !mapping.get(propertyKey).equals(neoKey)) {
-                    throw new IllegalArgumentException(formatWithLocale(
-                        "Specifying multiple neoPropertyKeys for the same property is not allowed, found propertyKey: %s, neoPropertyKeys: %s, %s.",
-                        propertyKey,
-                        neoKey,
-                        mapping.get(propertyKey)
-                    ));
+
+                if (seenMappings.containsKey(propertyKey)) {
+                    // we have another mapping with the same GDS key
+                    var seenMapping = seenMappings.get(propertyKey);
+
+                    if (!Objects.equals(seenMapping.neoPropertyKey(), propertyMapping.neoPropertyKey())) {
+                        throw new IllegalArgumentException(formatWithLocale(
+                            "Specifying multiple neoPropertyKeys for the same property is not allowed, " +
+                            "found propertyKey: `%s` with conflicting neoPropertyKeys: `%s`, `%s`.",
+                            propertyKey,
+                            propertyMapping.neoPropertyKey(),
+                            seenMapping.neoPropertyKey()
+                        ));
+                    }
+
+                    if (!Objects.equals(seenMapping.defaultValue(), propertyMapping.defaultValue())) {
+                        throw new IllegalArgumentException(formatWithLocale(
+                            "Specifying different default values for the same property with identical neoPropertyKey is not allowed, " +
+                            "found propertyKey: `%s` with conflicting default values: `%s`, `%s`.",
+                            propertyKey,
+                            propertyMapping.defaultValue().getObject(),
+                            seenMapping.defaultValue().getObject()
+                        ));
+                    }
                 }
-                mapping.put(propertyKey, neoKey);
+
+                seenMappings.put(propertyKey, propertyMapping);
             });
     }
 }

graph-projection-api/src/test/java/org/neo4j/gds/NodeProjectionsTest.java

@@ -147,37 +147,71 @@ void shouldFailOnDuplicatePropertyKeys() {
             .hasMessage("Duplicate property key `prop`");
     }
 
-    @Test
-    void shouldFailOnAmbiguousNodePropertyDefinition() {
-        var builder = NodeProjections.builder().projections(Map.of(
+    static Stream<Arguments> ambiguousPropertyMappings() {
+        return Stream.of(
+            Arguments.of(
+                "different neo key",
+                PropertyMapping.of("foo", "bar", DefaultValue.DEFAULT),
+                PropertyMapping.of("foo", "baz", DefaultValue.DEFAULT),
+                new String[]{
+                    "Specifying multiple neoPropertyKeys for the same property is not allowed, found propertyKey: `foo` with conflicting neoPropertyKeys:",
+                    "`bar`",
+                    "`baz`"
+                }
+            ),
+            Arguments.of(
+                "different default value types",
+                PropertyMapping.of("foo", "baz", DefaultValue.forLong()),
+                PropertyMapping.of("foo", "baz", DefaultValue.forDouble()),
+                new String[]{
+                    "Specifying different default values for the same property with identical neoPropertyKey is not allowed, found propertyKey: `foo` with conflicting default values:",
+                    "`-9223372036854775808`",
+                    "`NaN`",
+                }
+            ),
+            Arguments.of(
+                "different default values",
+                PropertyMapping.of("foo", "baz", DefaultValue.of(42)),
+                PropertyMapping.of("foo", "baz", DefaultValue.of(1337)),
+                new String[]{
+                    "Specifying different default values for the same property with identical neoPropertyKey is not allowed, found propertyKey: `foo` with conflicting default values:",
+                    "`42`",
+                    "`1337`",
+                }
+            )
+        );
+    }
+
+    @ParameterizedTest(name = "{0}")
+    @MethodSource("ambiguousPropertyMappings")
+    void shouldFailOnAmbiguousNodePropertyDefinition(
+        String ignore,
+        PropertyMapping first,
+        PropertyMapping second,
+        String... messages
+    ) {
+        var builder = ImmutableNodeProjections.builder().projections(Map.of(
             NodeLabel.of("A"),
             NodeProjection
                 .builder()
                 .label("A")
-                .properties(PropertyMappings
-                    .builder()
-                    .addMapping(PropertyMapping.of("foo", "bar", DefaultValue.DEFAULT))
-                    .build()
-                )
+                .addProperty(first)
                 .build(),
 
             NodeLabel.of("B"),
             NodeProjection
                 .builder()
                 .label("B")
-                .properties(PropertyMappings
-                    .builder()
-                    .addMapping(PropertyMapping.of("foo", "baz", DefaultValue.DEFAULT))
-                    .build()
-                )
+                .addProperty(second)
                 .build()
 
         ));
 
-        assertThatThrownBy(builder::build)
-            .isInstanceOf(IllegalArgumentException.class)
-            .hasMessageContaining(
-                "Specifying multiple neoPropertyKeys for the same property is not allowed, found propertyKey: foo, neoPropertyKeys");
+        var throwableAssert = assertThatThrownBy(builder::build)
+            .isInstanceOf(IllegalArgumentException.class);
+        for (var message : messages) {
+            throwableAssert.hasMessageContaining(message);
+        }
     }
 
     @Test