Skip to content

Commit ebe3b1f

Browse files
FlorentinDFlorentinD
authored
Merge pull request #5380 from Mats-SX/graphstore-quickwins
Smaller improvements to GraphStore
2 parents f9b5486 + 896a064 commit ebe3b1f

File tree

7 files changed

+148
-43
lines changed

7 files changed

+148
-43
lines changed

‎core/src/main/java/org/neo4j/gds/core/loading/CSRGraphStore.java‎

Lines changed: 35 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -49,12 +49,10 @@
4949
import org.neo4j.gds.api.schema.RelationshipPropertySchema;
5050
import org.neo4j.gds.api.schema.RelationshipSchema;
5151
import org.neo4j.gds.core.Aggregation;
52-
import org.neo4j.gds.core.ProcedureConstants;
5352
import org.neo4j.gds.core.huge.CSRCompositeRelationshipIterator;
5453
import org.neo4j.gds.core.huge.HugeGraph;
5554
import org.neo4j.gds.core.huge.NodeFilteredGraph;
5655
import org.neo4j.gds.core.huge.UnionGraph;
57-
import org.neo4j.gds.core.loading.construction.GraphFactory;
5856
import org.neo4j.gds.core.utils.TimeUtil;
5957
import org.neo4j.gds.utils.ExceptionUtil;
6058
import org.neo4j.gds.utils.StringJoining;
@@ -115,18 +113,13 @@ public static CSRGraphStore of(
115113
Map<RelationshipType, RelationshipPropertyStore> relationshipPropertyStores,
116114
int concurrency
117115
) {
118-
// A graph store must contain at least one topology, even if it is empty.
119-
var topologies = relationships.isEmpty()
120-
? Map.of(RelationshipType.ALL_RELATIONSHIPS, GraphFactory.emptyRelationships(nodes).topology())
121-
: relationships;
122-
123116
return new CSRGraphStore(
124117
databaseId,
125118
capabilities,
126119
schema,
127120
nodes,
128121
nodePropertyStore == null ? NodePropertyStore.empty() : nodePropertyStore,
129-
topologies,
122+
relationships,
130123
relationshipPropertyStores,
131124
concurrency
132125
);
@@ -448,9 +441,7 @@ public DeletionResult deleteRelationships(RelationshipType relationshipType) {
448441
return DeletionResult.of(builder ->
449442
updateGraphStore(graphStore -> {
450443
var removedTopology = graphStore.relationships.remove(relationshipType);
451-
if (removedTopology != null) {
452-
builder.deletedRelationships(removedTopology.elementCount());
453-
}
444+
builder.deletedRelationships(removedTopology == null ? 0 : removedTopology.elementCount());
454445

455446
var removedProperties = graphStore.relationshipProperties.remove(relationshipType);
456447

@@ -480,27 +471,8 @@ public DeletionResult deleteRelationships(RelationshipType relationshipType) {
480471
}
481472

482473
@Override
483-
public Graph getGraph(Collection<NodeLabel> nodeLabels) {
484-
var filteredNodes = getFilteredIdMap(nodeLabels);
485-
var filteredNodeProperties = filterNodeProperties(nodeLabels);
486-
487-
var graphSchema = GraphSchema.of(
488-
schema().nodeSchema(),
489-
RelationshipSchema.empty(),
490-
schema.graphProperties()
491-
);
492-
493-
var initialGraph = HugeGraph.create(
494-
nodes,
495-
graphSchema,
496-
filteredNodeProperties,
497-
Relationships.Topology.EMPTY,
498-
Optional.empty()
499-
);
500-
501-
return filteredNodes.isPresent()
502-
? new NodeFilteredGraph(initialGraph, filteredNodes.get())
503-
: initialGraph;
474+
public CSRGraph getGraph(Collection<NodeLabel> nodeLabels) {
475+
return getGraph(nodeLabels, List.of(), Optional.empty());
504476
}
505477

506478
@Override
@@ -510,11 +482,18 @@ public CSRGraph getGraph(
510482
Optional<String> maybeRelationshipProperty
511483
) {
512484
validateInput(relationshipTypes, maybeRelationshipProperty);
513-
return createGraph(nodeLabels, relationshipTypes, maybeRelationshipProperty);
485+
if (relationshipTypes.isEmpty()) {
486+
return createNodeOnlyGraph(nodeLabels);
487+
} else {
488+
return createGraph(nodeLabels, relationshipTypes, maybeRelationshipProperty);
489+
}
514490
}
515491

516492
@Override
517493
public CSRGraph getUnion() {
494+
if (relationships.isEmpty()) {
495+
return getGraph(nodeLabels());
496+
}
518497
var graphs = relationships
519498
.keySet()
520499
.stream()
@@ -683,6 +662,29 @@ private CSRGraph createGraph(
683662
return UnionGraph.of(filteredGraphs);
684663
}
685664

665+
private CSRGraph createNodeOnlyGraph(Collection<NodeLabel> nodeLabels) {
666+
var filteredNodes = getFilteredIdMap(nodeLabels);
667+
var filteredNodeProperties = filterNodeProperties(nodeLabels);
668+
669+
var graphSchema = GraphSchema.of(
670+
schema().nodeSchema(),
671+
RelationshipSchema.empty(),
672+
schema.graphProperties()
673+
);
674+
675+
var initialGraph = HugeGraph.create(
676+
nodes,
677+
graphSchema,
678+
filteredNodeProperties,
679+
Relationships.Topology.EMPTY,
680+
Optional.empty()
681+
);
682+
683+
return filteredNodes.isPresent()
684+
? new NodeFilteredGraph(initialGraph, filteredNodes.get())
685+
: initialGraph;
686+
}
687+
686688
@NotNull
687689
private Optional<IdMap> getFilteredIdMap(Collection<NodeLabel> filteredLabels) {
688690
boolean loadAllNodes = filteredLabels.containsAll(nodeLabels());
@@ -746,13 +748,6 @@ private void validateInput(
746748
Collection<RelationshipType> relationshipTypes,
747749
Optional<String> maybeRelationshipProperty
748750
) {
749-
if (relationshipTypes.isEmpty()) {
750-
throw new IllegalArgumentException(formatWithLocale(
751-
"The parameter '%s' should not be empty. Use '*' to load all relationship types.",
752-
ProcedureConstants.RELATIONSHIP_TYPES
753-
));
754-
}
755-
756751
relationshipTypes.forEach(relationshipType -> {
757752
if (!relationships.containsKey(relationshipType)) {
758753
throw new IllegalArgumentException(formatWithLocale(

‎core/src/test/java/org/neo4j/gds/api/schema/NodeSchemaTest.java‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,11 +34,18 @@
3434

3535
import static org.assertj.core.api.Assertions.assertThat;
3636
import static org.junit.jupiter.api.Assertions.assertEquals;
37+
import static org.junit.jupiter.api.Assertions.assertFalse;
3738
import static org.junit.jupiter.api.Assertions.assertThrows;
3839
import static org.junit.jupiter.api.Assertions.assertTrue;
3940

4041
class NodeSchemaTest {
4142

43+
@Test
44+
void handlesOutsideOfSchemaRequests() {
45+
NodeSchema empty = NodeSchema.builder().build();
46+
assertFalse(empty.hasProperty(NodeLabel.of("NotInSchema"), "notInSchemaEither"));
47+
}
48+
4249
@Test
4350
void testDefaultValues() {
4451
var label = NodeLabel.of("Foo");

‎core/src/test/java/org/neo4j/gds/api/schema/RelationshipSchemaTest.java‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,11 +34,18 @@
3434

3535
import static org.assertj.core.api.Assertions.assertThat;
3636
import static org.junit.jupiter.api.Assertions.assertEquals;
37+
import static org.junit.jupiter.api.Assertions.assertFalse;
3738
import static org.junit.jupiter.api.Assertions.assertThrows;
3839
import static org.junit.jupiter.api.Assertions.assertTrue;
3940

4041
class RelationshipSchemaTest {
4142

43+
@Test
44+
void handlesOutsideOfSchemaRequests() {
45+
var empty = RelationshipSchema.builder().build();
46+
assertFalse(empty.hasProperty(RelationshipType.of("NotInSchema"), "notInSchemaEither"));
47+
}
48+
4249
@Test
4350
void testDefaultValuesAndAggregation() {
4451
var relType = RelationshipType.of("BAR");

‎core/src/test/java/org/neo4j/gds/core/loading/CSRGraphStoreTest.java‎

Lines changed: 85 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,85 @@
1+
/*
2+
* Copyright (c) "Neo4j"
3+
* Neo4j Sweden AB [http://neo4j.com]
4+
*
5+
* This file is part of Neo4j.
6+
*
7+
* Neo4j is free software: you can redistribute it and/or modify
8+
* it under the terms of the GNU General Public License as published by
9+
* the Free Software Foundation, either version 3 of the License, or
10+
* (at your option) any later version.
11+
*
12+
* This program is distributed in the hope that it will be useful,
13+
* but WITHOUT ANY WARRANTY; without even the implied warranty of
14+
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
15+
* GNU General Public License for more details.
16+
*
17+
* You should have received a copy of the GNU General Public License
18+
* along with this program. If not, see <http://www.gnu.org/licenses/>.
19+
*/
20+
package org.neo4j.gds.core.loading;
21+
22+
import org.junit.jupiter.api.Test;
23+
import org.neo4j.gds.NodeLabel;
24+
import org.neo4j.gds.RelationshipType;
25+
import org.neo4j.gds.gdl.GdlFactory;
26+
27+
import java.util.List;
28+
import java.util.Optional;
29+
30+
import static org.assertj.core.api.Assertions.assertThat;
31+
import static org.assertj.core.api.Assertions.assertThatThrownBy;
32+
import static org.neo4j.gds.TestSupport.assertGraphEquals;
33+
import static org.neo4j.gds.TestSupport.fromGdl;
34+
35+
class CSRGraphStoreTest {
36+
37+
@Test
38+
void deleteAdditionalRelationshipTypes() {
39+
GdlFactory factory = GdlFactory.of("(b)-[:REL {x: 1}]->(a), (b)-[:REL]->(c)");
40+
var graphStore = factory.build();
41+
42+
var del1 = graphStore.deleteRelationships(RelationshipType.of("REL"));
43+
44+
assertThat(del1.deletedRelationships()).isEqualTo(2);
45+
assertThat(del1.deletedProperties()).containsEntry("x", 2L).hasSize(1);
46+
47+
var del2 = graphStore.deleteRelationships(RelationshipType.of("REL"));
48+
49+
assertThat(del2.deletedRelationships()).isEqualTo(0);
50+
assertThat(del2.deletedProperties()).isEmpty();
51+
}
52+
53+
@Test
54+
void validateRelationshipTypesWhenNoneExist() {
55+
GdlFactory factory = GdlFactory.of("(a), (b)");
56+
var graphStore = factory.build();
57+
58+
assertThatThrownBy(() -> graphStore.getGraph(
59+
List.of(NodeLabel.ALL_NODES),
60+
List.of(RelationshipType.of("X")),
61+
Optional.empty()
62+
)).hasMessageContaining("No relationships have been loaded for relationship type").hasMessageContaining("X");
63+
}
64+
65+
@Test
66+
void gettingGraphsWithRelationshipTypes() {
67+
GdlFactory factory = GdlFactory.of("()-[:T]->()-[:R]->()-[:R]->()");
68+
var graphStore = factory.build();
69+
70+
var t_graph = graphStore.getGraph(graphStore.nodeLabels(), List.of(RelationshipType.of("T")), Optional.empty());
71+
var r_graph = graphStore.getGraph(graphStore.nodeLabels(), List.of(RelationshipType.of("R")), Optional.empty());
72+
var t_r_graph = graphStore.getGraph(
73+
graphStore.nodeLabels(),
74+
List.of(RelationshipType.of("R"), RelationshipType.of("T")),
75+
Optional.empty()
76+
);
77+
var none_graph = graphStore.getGraph(graphStore.nodeLabels(), List.of(), Optional.empty());
78+
79+
assertGraphEquals(fromGdl("()-[:T]->(), (), ()"), t_graph);
80+
assertGraphEquals(fromGdl("(), ()-[:R]->()-[:R]->()"), r_graph);
81+
assertGraphEquals(fromGdl("()-[:T]->()-[:R]->()-[:R]->()"), t_r_graph);
82+
assertGraphEquals(fromGdl("(), (), (), ()"), none_graph);
83+
}
84+
85+
}

‎graph-schema-api/src/main/java/org/neo4j/gds/api/schema/ElementSchema.java‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -59,7 +59,9 @@ default boolean hasProperties(ELEMENT_IDENTIFIER elementIdentifier) {
5959

6060
@Value.Default
6161
default boolean hasProperty(ELEMENT_IDENTIFIER elementIdentifier, String propertyKey) {
62-
return (!properties().get(elementIdentifier).isEmpty() && properties().get(elementIdentifier).containsKey(propertyKey));
62+
return properties().containsKey(elementIdentifier) && (!properties()
63+
.get(elementIdentifier)
64+
.isEmpty() && properties().get(elementIdentifier).containsKey(propertyKey));
6365
}
6466

6567
@Value.Default

‎proc/community/src/test/java/org/neo4j/gds/wcc/WccStatsProcTest.java‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,15 @@ public WccStatsConfig createConfig(CypherMapWrapper mapWrapper) {
5050
return WccStatsConfig.of(mapWrapper);
5151
}
5252

53+
@Test
54+
void testRelationshipTypesEmpty() {
55+
runQuery("CALL gds.graph.project('g', '*', '*')");
56+
57+
assertCypherResult("CALL gds.wcc.stats('g', {relationshipTypes: []}) YIELD componentCount", List.of(Map.of(
58+
"componentCount", 10L
59+
)));
60+
}
61+
5362
@Test
5463
void yields() {
5564
loadGraph(DEFAULT_GRAPH_NAME);

‎test-utils/src/test/java/org/neo4j/gds/extension/TestGraphTest.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -69,7 +69,7 @@ void usesIdFunctionForOriginalId() {
6969
CSRGraph bGraph = GdlFactory
7070
.of("(:A), (b:B), (:B)")
7171
.build()
72-
.getGraph(List.of(NodeLabel.of("B")), List.of(RelationshipType.ALL_RELATIONSHIPS), Optional.empty());
72+
.getGraph(List.of(NodeLabel.of("B")), List.of(), Optional.empty());
7373
TestGraph g = new TestGraph(bGraph, (a) -> a.equals("b") ? 1 : 2, "foo");
7474
assertEquals(1, g.toOriginalNodeId("b"));
7575
assertEquals(2, g.toOriginalNodeId("notB"));
@@ -80,7 +80,7 @@ void usesInnerGraphForMappedId() {
8080
CSRGraph bGraph = GdlFactory
8181
.of("(:A), (b:B), (:B)")
8282
.build()
83-
.getGraph(List.of(NodeLabel.of("B")), List.of(RelationshipType.ALL_RELATIONSHIPS), Optional.empty());
83+
.getGraph(List.of(NodeLabel.of("B")), List.of(), Optional.empty());
8484
TestGraph g = new TestGraph(bGraph, (a) -> a.equals("b") ? 1 : 2, "foo");
8585
assertEquals(0, g.toMappedNodeId("b"));
8686
assertEquals(1, g.toMappedNodeId("notB"));

0 commit comments

Comments
 (0)