Merge pull request #9321 from FlorentinD/cypher-proj-unused-reltypes-…

…validation

Throw on not projected undirected/inverseIndexed relTypes

cypher-aggregation/src/integrationTest/java/org/neo4j/gds/projection/CypherAggregationTest.java

@@ -1152,6 +1152,51 @@ void shouldFailOnEmptyGraphName() {
             .withMessageContaining("`graphName` can not be null or blank");
     }
 
+    @Test
+    void shouldFailOnInvalidUndirectedRelationshipTypes() {
+        var query =
+            """
+                 MATCH (s)-[r]->(t)
+                 RETURN gds.graph.project(
+                    'g',
+                    s,
+                    t,
+                    {relationshipType: type(r)},
+                    {undirectedRelationshipTypes: ['REL', 'invalidRel']}
+                )
+                """;
+
+        assertThatException()
+            .isThrownBy(() -> runQuery(query))
+            .withMessageContaining(
+                "Specified undirectedRelationshipTypes `[invalidRel]` were not projected in the graph." +
+                    " Projected types are: `['DISCONNECTED', 'REL']`."
+            );
+    }
+
+    @Test
+    void shouldFailOnInvalidInverseRelationshipTypes() {
+        var query =
+            """
+                 MATCH (s)-[r]->(t)
+                 RETURN gds.graph.project(
+                    'g',
+                    s,
+                    t,
+                    {relationshipType: type(r)},
+                    {inverseIndexedRelationshipTypes: ['REL', 'invalidRel']}
+                )
+                """;
+
+        assertThatException()
+            .isThrownBy(() -> runQuery(query))
+            .withMessageContaining(
+                "Specified inverseIndexedRelationshipTypes `[invalidRel]` were not projected in the graph." +
+                    " Projected types are: `['DISCONNECTED', 'REL']`."
+            );
+    }
+
+
     @Nested
     class LargerGraphTest extends RandomGraphTestCase {
 

doc/modules/ROOT/pages/machine-learning/node-embeddings/graph-sage.adoc

@@ -534,9 +534,10 @@ RETURN gds.graph.project(
     sourceNodeProperties: source { .age, .heightAndWeight },
     targetNodeLabels: labels(target),
     targetNodeProperties: target { .cost },
+    relationshipType: type(r),
     relationshipProperties: r { .relWeight }
   },
-  { undirectedRelationshipTypes: ['KNOWS', 'LIKES'] }
+  { undirectedRelationshipTypes: ['LIKES'] }
 )
 ----
 

triplet-graph-builder/src/main/java/org/neo4j/gds/projection/GraphImporter.java

@@ -20,13 +20,15 @@
 package org.neo4j.gds.projection;
 
 import org.jetbrains.annotations.Nullable;
+import org.neo4j.gds.ElementProjection;
 import org.neo4j.gds.RelationshipType;
 import org.neo4j.gds.api.DatabaseInfo;
 import org.neo4j.gds.api.DefaultValue;
 import org.neo4j.gds.api.compress.AdjacencyCompressor;
 import org.neo4j.gds.api.schema.ImmutableMutableGraphSchema;
 import org.neo4j.gds.api.schema.MutableGraphSchema;
 import org.neo4j.gds.api.schema.MutableRelationshipSchema;
+import org.neo4j.gds.api.schema.RelationshipSchema;
 import org.neo4j.gds.config.GraphProjectConfig;
 import org.neo4j.gds.core.Aggregation;
 import org.neo4j.gds.core.loading.Capabilities.WriteMode;
@@ -42,11 +44,15 @@
 import org.neo4j.gds.core.loading.construction.PropertyValues;
 import org.neo4j.gds.core.loading.construction.RelationshipsBuilder;
 import org.neo4j.gds.core.utils.ProgressTimer;
+import org.neo4j.gds.utils.StringJoining;
 
 import java.util.List;
+import java.util.Locale;
 import java.util.Map;
 import java.util.Optional;
+import java.util.Set;
 import java.util.concurrent.ConcurrentHashMap;
+import java.util.stream.Collectors;
 
 import static java.util.stream.Collectors.toMap;
 import static org.neo4j.gds.Orientation.NATURAL;
@@ -159,6 +165,7 @@ public AggregationResult result(
         buildRelationshipsWithProperties(graphStoreBuilder, valueMapper);
 
         var graphStore = graphStoreBuilder.schema(this.graphSchemaBuilder.build()).build();
+        validateRelTypes(graphStore.schema().relationshipSchema());
 
         GraphStoreCatalog.set(this.config, graphStore);
 
@@ -180,101 +187,135 @@ public AggregationResult result(
             .build();
     }
 
-    private RelationshipsBuilder newRelImporter(RelationshipType relType, @Nullable PropertyValues properties) {
-        var orientation = this.undirectedRelationshipTypes.contains(relType.name) || this.undirectedRelationshipTypes
-            .contains(
-                "*"
-            )
-                ? UNDIRECTED
-                : NATURAL;
-
-        boolean indexInverse = inverseIndexedRelationshipTypes.contains(relType.name) || inverseIndexedRelationshipTypes
-            .contains("*");
-
-        var relationshipsBuilderBuilder = GraphFactory.initRelationshipsBuilder()
-            .nodes(this.idMapBuilder)
-            .relationshipType(relType)
-            .orientation(orientation)
-            .aggregation(Aggregation.NONE)
-            .indexInverse(indexInverse)
-            .concurrency(this.config.readConcurrency())
-            .usePooledBuilderProvider(true);
+    private void validateRelTypes(RelationshipSchema relationshipSchema) {
+        var  unusedUndirectedTypes = notProjectedRelationshipTypes(relationshipSchema, undirectedRelationshipTypes);
+        if (!unusedUndirectedTypes.isEmpty()) {
+            throw new IllegalArgumentException(String.format(Locale.US,
+                "Specified undirectedRelationshipTypes `%s` were not projected in the graph. " + "Projected types are: `%s`.",
+                unusedUndirectedTypes,
+                StringJoining.join(relationshipSchema.availableTypes().stream().map(RelationshipType::name))
+            ));
+        }
+        var unusedInverseTypes = notProjectedRelationshipTypes(relationshipSchema, inverseIndexedRelationshipTypes);
+        if (!unusedInverseTypes.isEmpty()) {
+            throw new IllegalArgumentException(String.format(
+                Locale.US,
+                "Specified inverseIndexedRelationshipTypes `%s` were not projected in the graph. " + "Projected types are: `%s`.",
+                unusedInverseTypes,
+                StringJoining.join(relationshipSchema.availableTypes().stream().map(RelationshipType::name))
+            ));
+        }
+    }
 
-        if (properties != null) {
-            for (String propertyKey : properties.propertyKeys()) {
-                relationshipsBuilderBuilder.addPropertyConfig(
-                    ImmutablePropertyConfig.builder().propertyKey(propertyKey).build()
-                );
-            }
+    private List<String> notProjectedRelationshipTypes(RelationshipSchema schema, List<String> givenRelationshipTypes) {
+        if (givenRelationshipTypes.contains(ElementProjection.PROJECT_ALL)) {
+            return List.of();
         }
 
-        return relationshipsBuilderBuilder.build();
+        Set<String> typesInSchema = schema.availableTypes().stream()
+            .map(RelationshipType::name)
+            .collect(Collectors.toSet());
+
+        return givenRelationshipTypes.stream()
+            .filter(type -> !typesInSchema.contains(type))
+            .toList();
     }
 
-    /**
-     * Adds the given node to the internal nodes builder and returns
-     * the intermediate node id which can be used for relationships.
-     *
-     * @return intermediate node id
-     */
-    private long loadNode(
-        long node,
-        NodeLabelToken nodeLabels,
-        @Nullable PropertyValues nodeProperties
-    ) {
-        return nodeProperties == null
-            ? this.idMapBuilder.addNode(node, nodeLabels)
-            : this.idMapBuilder.addNodeWithProperties(
-                node,
-                nodeProperties,
-                nodeLabels
+    private RelationshipsBuilder newRelImporter(RelationshipType relType, @Nullable PropertyValues properties) {
+    var orientation = this.undirectedRelationshipTypes.contains(relType.name) || this.undirectedRelationshipTypes
+        .contains(
+            "*"
+        )
+        ? UNDIRECTED
+        : NATURAL;
+
+    boolean indexInverse = inverseIndexedRelationshipTypes.contains(relType.name) || inverseIndexedRelationshipTypes
+        .contains("*");
+
+    var relationshipsBuilderBuilder = GraphFactory.initRelationshipsBuilder()
+        .nodes(this.idMapBuilder)
+        .relationshipType(relType)
+        .orientation(orientation)
+        .aggregation(Aggregation.NONE)
+        .indexInverse(indexInverse)
+        .concurrency(this.config.readConcurrency())
+        .usePooledBuilderProvider(true);
+
+    if (properties != null) {
+        for (String propertyKey : properties.propertyKeys()) {
+            relationshipsBuilderBuilder.addPropertyConfig(
+                ImmutablePropertyConfig.builder().propertyKey(propertyKey).build()
             );
+        }
     }
 
-    private AdjacencyCompressor.ValueMapper buildNodesWithProperties(GraphStoreBuilder graphStoreBuilder) {
-        var idMapAndProperties = this.idMapBuilder.build();
-
-        var idMap = idMapAndProperties.idMap();
-        var nodeSchema = idMapAndProperties.schema();
+    return relationshipsBuilderBuilder.build();
+}
 
-        this.graphSchemaBuilder.nodeSchema(nodeSchema);
+/**
+ * Adds the given node to the internal nodes builder and returns
+ * the intermediate node id which can be used for relationships.
+ *
+ * @return intermediate node id
+ */
+private long loadNode(
+    long node,
+    NodeLabelToken nodeLabels,
+    @Nullable PropertyValues nodeProperties
+) {
+    return nodeProperties == null
+        ? this.idMapBuilder.addNode(node, nodeLabels)
+        : this.idMapBuilder.addNodeWithProperties(
+            node,
+            nodeProperties,
+            nodeLabels
+        );
+}
 
-        var nodes = ImmutableNodes
-            .builder()
-            .idMap(idMap)
-            .schema(nodeSchema)
-            .properties(idMapAndProperties.propertyStore())
-            .build();
+private AdjacencyCompressor.ValueMapper buildNodesWithProperties(GraphStoreBuilder graphStoreBuilder) {
+    var idMapAndProperties = this.idMapBuilder.build();
 
-        graphStoreBuilder.nodes(nodes);
+    var idMap = idMapAndProperties.idMap();
+    var nodeSchema = idMapAndProperties.schema();
 
-        // Relationships are added using their intermediate node ids.
-        // In order to map to the final internal ids, we need to use
-        // the mapping function of the wrapped id map.
-        return idMap.rootIdMap()::toMappedNodeId;
-    }
+    this.graphSchemaBuilder.nodeSchema(nodeSchema);
 
-    private void buildRelationshipsWithProperties(
-        GraphStoreBuilder graphStoreBuilder,
-        AdjacencyCompressor.ValueMapper valueMapper
-    ) {
-        var relationshipImportResultBuilder = RelationshipImportResult.builder();
+    var nodes = ImmutableNodes
+        .builder()
+        .idMap(idMap)
+        .schema(nodeSchema)
+        .properties(idMapAndProperties.propertyStore())
+        .build();
 
-        var relationshipSchema = MutableRelationshipSchema.empty();
-        this.relImporters.forEach((relationshipType, relImporter) -> {
-            var relationships = relImporter.build(
-                Optional.of(valueMapper),
-                Optional.empty()
-            );
-            relationshipSchema.set(relationships.relationshipSchemaEntry());
-            relationshipImportResultBuilder.putImportResult(relationshipType, relationships);
-        });
+    graphStoreBuilder.nodes(nodes);
 
-        graphStoreBuilder.relationshipImportResult(relationshipImportResultBuilder.build());
-        this.graphSchemaBuilder.relationshipSchema(relationshipSchema);
+    // Relationships are added using their intermediate node ids.
+    // In order to map to the final internal ids, we need to use
+    // the mapping function of the wrapped id map.
+    return idMap.rootIdMap()::toMappedNodeId;
+}
 
-        // release all references to the builders
-        // we are only be called once and don't support double invocations of `result` building
-        this.relImporters.clear();
-    }
+private void buildRelationshipsWithProperties(
+    GraphStoreBuilder graphStoreBuilder,
+    AdjacencyCompressor.ValueMapper valueMapper
+) {
+    var relationshipImportResultBuilder = RelationshipImportResult.builder();
+
+    var relationshipSchema = MutableRelationshipSchema.empty();
+    this.relImporters.forEach((relationshipType, relImporter) -> {
+        var relationships = relImporter.build(
+            Optional.of(valueMapper),
+            Optional.empty()
+        );
+        relationshipSchema.set(relationships.relationshipSchemaEntry());
+        relationshipImportResultBuilder.putImportResult(relationshipType, relationships);
+    });
+
+    graphStoreBuilder.relationshipImportResult(relationshipImportResultBuilder.build());
+    this.graphSchemaBuilder.relationshipSchema(relationshipSchema);
+
+    // release all references to the builders
+    // we are only be called once and don't support double invocations of `result` building
+    this.relImporters.clear();
+}
 }

triplet-graph-builder/src/test/java/org/neo4j/gds/projection/GraphImporterTest.java

@@ -39,6 +39,7 @@
 import java.util.Map;
 
 import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
 import static org.neo4j.gds.TestSupport.assertGraphEquals;
 import static org.neo4j.gds.TestSupport.fromGdl;
 
@@ -309,4 +310,64 @@ void shouldImportRelationshipsWithProperties() {
             graphStore.getUnion()
         );
     }
+
+    @Test
+    void shouldFailImportWithUnusedUndirectedRelationshipType() {
+        var importer = new GraphImporter(GraphProjectConfig.emptyWithName("", "g"),
+            List.of("UNUSED_REL"),
+            List.of(),
+            new LazyIdMapBuilderBuilder().concurrency(new Concurrency(4))
+                .hasLabelInformation(true)
+                .hasProperties(true)
+                .propertyState(PropertyState.REMOTE)
+                .build(),
+            Capabilities.WriteMode.REMOTE,
+            ""
+        );
+
+        importer.update(0,
+            1,
+            null,
+            null,
+            NodeLabelTokens.empty(),
+            NodeLabelTokens.empty(),
+            RelationshipType.of("REL"),
+            null
+        );
+
+        assertThatThrownBy(() -> importer.result(DatabaseInfo.of(DatabaseId.EMPTY, DatabaseInfo.DatabaseLocation.LOCAL),
+            ProgressTimer.start(),
+            true
+        )).hasMessage("Specified undirectedRelationshipTypes `[UNUSED_REL]` were not projected in the graph. Projected types are: `['REL']`.");
+    }
+
+    @Test
+    void shouldFailImportWithUnusedInverseRelationshipType() {
+        var importer = new GraphImporter(GraphProjectConfig.emptyWithName("", "g"),
+            List.of(),
+            List.of("UNUSED_REL"),
+            new LazyIdMapBuilderBuilder().concurrency(new Concurrency(4))
+                .hasLabelInformation(true)
+                .hasProperties(true)
+                .propertyState(PropertyState.REMOTE)
+                .build(),
+            Capabilities.WriteMode.REMOTE,
+            ""
+        );
+
+        importer.update(0,
+            1,
+            null,
+            null,
+            NodeLabelTokens.empty(),
+            NodeLabelTokens.empty(),
+            RelationshipType.of("REL"),
+            null
+        );
+
+        assertThatThrownBy(() -> importer.result(DatabaseInfo.of(DatabaseId.EMPTY, DatabaseInfo.DatabaseLocation.LOCAL),
+            ProgressTimer.start(),
+            true
+        )).hasMessage("Specified inverseIndexedRelationshipTypes `[UNUSED_REL]` were not projected in the graph. Projected types are: `['REL']`.");
+    }
 }