Merge pull request #10563 from IoannisPanagiotas/knn-validation-preload

Move Validation out of algorithm and change exception being thrown

Author: IoannisPanagiotas

algo/src/main/java/org/neo4j/gds/similarity/knn/metrics/SimilarityComputer.java

@@ -25,10 +25,8 @@
 import org.neo4j.gds.api.properties.nodes.NodePropertyValues;
 import org.neo4j.gds.similarity.knn.KnnNodePropertySpec;
 import org.neo4j.gds.similarity.knn.metrics.LongArrayPropertySimilarityComputer.SortedLongArrayPropertyValues;
-import org.neo4j.gds.utils.StringJoining;
 
 import java.util.List;
-import java.util.Objects;
 
 import static org.neo4j.gds.utils.StringFormatting.formatWithLocale;
 
@@ -52,14 +50,7 @@ static SimilarityComputer ofProperties(Graph graph, List<KnnNodePropertySpec> kn
 
     static SimilarityComputer ofProperty(Graph graph, KnnNodePropertySpec knnNodePropertySpec) {
         var propertyName = knnNodePropertySpec.name();
-        var nodeProperties = Objects.requireNonNull(
-            graph.nodeProperties(propertyName),
-            () -> formatWithLocale(
-                "The property `%s` has not been loaded. Available properties: %s",
-                propertyName,
-                StringJoining.join(graph.availableNodeProperties())
-            )
-        );
+        var nodeProperties = graph.nodeProperties(propertyName);
 
         if (knnNodePropertySpec.metric() == SimilarityMetric.DEFAULT) {
             knnNodePropertySpec.setMetric(SimilarityMetric.defaultMetricForType(nodeProperties.valueType()));

algo/src/test/java/org/neo4j/gds/similarity/filteredknn/KnnBaseConfigTest.java

@@ -0,0 +1,80 @@
+/*
+ * Copyright (c) "Neo4j"
+ * Neo4j Sweden AB [http://neo4j.com]
+ *
+ * This file is part of Neo4j.
+ *
+ * Neo4j is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+package org.neo4j.gds.similarity.filteredknn;
+
+import org.junit.jupiter.api.Test;
+import org.neo4j.gds.api.GraphStore;
+import org.neo4j.gds.similarity.knn.KnnBaseConfig;
+import org.neo4j.gds.similarity.knn.KnnNodePropertySpec;
+
+import java.util.List;
+import java.util.Set;
+
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
+import static org.mockito.ArgumentMatchers.anyCollection;
+import static org.mockito.ArgumentMatchers.eq;
+import static org.mockito.Mockito.doCallRealMethod;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+ class KnnBaseConfigTest {
+
+    @Test
+    void shouldNotThrowForExistingProperty(){
+        var  graphStore = mock(GraphStore.class);
+        when(graphStore.hasNodeProperty("foo")).thenReturn(true);
+
+        var knnSpec = mock(KnnNodePropertySpec.class);
+        when(knnSpec.name()).thenReturn("foo");
+
+        List<KnnNodePropertySpec> props = List.of(knnSpec);
+
+        var config = mock(KnnBaseConfig.class);
+        when(config.nodeProperties()).thenReturn(props);
+
+        doCallRealMethod().when(config).checkPropertiesExist(eq(graphStore),anyCollection(),anyCollection());
+
+        assertDoesNotThrow( () -> config.checkPropertiesExist(graphStore,Set.of(),Set.of()));
+    }
+
+    @Test
+    void shouldThrowForNonExistingProperty(){
+        var  graphStore = mock(GraphStore.class);
+        when(graphStore.hasNodeProperty("foo")).thenReturn(true);
+        when(graphStore.hasNodeProperty("bar")).thenReturn(false);
+
+        when(graphStore.nodePropertyKeys(anyCollection())).thenReturn(Set.of("foo"));
+
+        var knnSpec1 = mock(KnnNodePropertySpec.class);
+        when(knnSpec1.name()).thenReturn("foo");
+        var knnSpec2 = mock(KnnNodePropertySpec.class);
+        when(knnSpec2.name()).thenReturn("bar");
+
+        List<KnnNodePropertySpec> props = List.of(knnSpec1, knnSpec2);
+
+        var config = mock(KnnBaseConfig.class);
+        when(config.nodeProperties()).thenReturn(props);
+        doCallRealMethod().when(config).checkPropertiesExist(eq(graphStore),anyCollection(),anyCollection());
+
+        assertThatThrownBy(() -> config.checkPropertiesExist(graphStore,Set.of(),Set.of()))
+            .hasMessageContaining("The property `bar` has not been loaded. Available properties: ['foo']");
+    }
+}

applications/algorithms/similarity/build.gradle

@@ -19,4 +19,5 @@ dependencies {
     // this is needed because NodeSimilarity internally calls Wcc
     implementation project(':community-algorithms')
     implementation project(":community-configs")
+
 }

applications/algorithms/similarity/src/main/java/org/neo4j/gds/applications/algorithms/similarity/SimilarityAlgorithmsMutateModeBusinessFacade.java

@@ -79,6 +79,7 @@ public <RESULT> RESULT filteredKnn(
             mutateStep,
             resultBuilder
         );
+
     }
 
     public <RESULT> RESULT filteredNodeSimilarity(

applications/algorithms/similarity/src/main/java/org/neo4j/gds/applications/algorithms/similarity/SimilarityAlgorithmsWriteModeBusinessFacade.java

@@ -78,6 +78,7 @@ public <RESULT> RESULT filteredKnn(
             (graph, __) -> similarityAlgorithms.filteredKnn(graph, configuration),
             writeStep,
             resultBuilder
+
         );
     }
 

proc/similarity/src/test/java/org/neo4j/gds/similarity/filteredknn/FilteredKnnMutateProcTest.java

@@ -36,6 +36,7 @@
 import java.util.List;
 
 import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
 import static org.assertj.core.api.InstanceOfAssertFactories.DOUBLE;
 import static org.assertj.core.api.InstanceOfAssertFactories.LONG;
 import static org.assertj.core.api.InstanceOfAssertFactories.MAP;
@@ -282,4 +283,11 @@ void shouldMutateWithFilteredNodesAndMultipleProperties() {
         );
     }
 
+    @Test
+    void shouldIllegalArgumentExceptionThrowForNullProperty(){
+        var query = "CALL gds.knn.filtered.mutate('filteredKnnGraph', {nodeProperties: ['foo'],  mutateProperty: 'foo', mutateRelationshipType: 'bar'}) YIELD *";
+        assertThatThrownBy(() -> runQuery(query))
+            .hasMessageContaining("Caused by: java.lang.IllegalArgumentException");
+    }
+
 }

proc/similarity/src/test/java/org/neo4j/gds/similarity/filteredknn/FilteredKnnStatsProcTest.java

@@ -21,6 +21,7 @@
 
 import org.junit.jupiter.api.AfterEach;
 import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
 import org.junit.jupiter.params.ParameterizedTest;
 import org.junit.jupiter.params.provider.Arguments;
 import org.junit.jupiter.params.provider.MethodSource;
@@ -36,6 +37,7 @@
 import java.util.stream.Stream;
 
 import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
 import static org.assertj.core.api.InstanceOfAssertFactories.DOUBLE;
 import static org.assertj.core.api.InstanceOfAssertFactories.LONG;
 import static org.assertj.core.api.InstanceOfAssertFactories.MAP;
@@ -143,4 +145,11 @@ private static Stream<Arguments> negativeGraphs() {
             Arguments.of("CREATE ({weight: -99}), ({weight: -10})", "negative long values")
         );
     }
+
+    @Test
+    void shouldIllegalArgumentExceptionThrowForNullProperty(){
+        var query = "CALL gds.knn.filtered.stats('filteredKnnGraph', {nodeProperties: ['foo']}) YIELD *";
+        assertThatThrownBy(() -> runQuery(query))
+            .hasMessageContaining("Caused by: java.lang.IllegalArgumentException");
+    }
 }

proc/similarity/src/test/java/org/neo4j/gds/similarity/filteredknn/FilteredKnnStreamProcTest.java

@@ -36,6 +36,8 @@
 import java.util.Map;
 import java.util.stream.Collectors;
 
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+
 class FilteredKnnStreamProcTest extends BaseProcTest {
 
     @Neo4jGraph
@@ -211,4 +213,11 @@ void shouldEmployTargetNodeFilter() {
             Map.of("node1", idMap.get("dave"), "node2", idMap.get("bob"), "similarity", 1.0)
         ));
     }
+
+    @Test
+    void shouldIllegalArgumentExceptionThrowForNullProperty(){
+        var query = "CALL gds.knn.filtered.stream('filteredKnnGraph', {nodeProperties: ['foo']}) YIELD *";
+        assertThatThrownBy(() -> runQuery(query))
+            .hasMessageContaining("Caused by: java.lang.IllegalArgumentException");
+    }
 }

proc/similarity/src/test/java/org/neo4j/gds/similarity/filteredknn/FilteredKnnWriteProcTest.java

@@ -39,6 +39,7 @@
 import java.util.List;
 
 import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
 import static org.assertj.core.api.InstanceOfAssertFactories.DOUBLE;
 import static org.assertj.core.api.InstanceOfAssertFactories.LONG;
 import static org.assertj.core.api.InstanceOfAssertFactories.MAP;
@@ -219,4 +220,11 @@ void shouldWriteWithFilteredNodes() {
             knnGraph
         );
     }
+
+    @Test
+    void shouldIllegalArgumentExceptionThrowForNullProperty(){
+        var query = "CALL gds.knn.filtered.write('filteredKnnGraph', {nodeProperties: ['foo'],  writeProperty: 'foo', writeRelationshipType: 'bar'}) YIELD *";
+        assertThatThrownBy(() -> runQuery(query))
+            .hasMessageContaining("Caused by: java.lang.IllegalArgumentException");
+    }
 }

proc/similarity/src/test/java/org/neo4j/gds/similarity/knn/KnnMutateProcTest.java

@@ -34,6 +34,7 @@
 import java.util.Map;
 
 import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
 import static org.assertj.core.api.InstanceOfAssertFactories.DOUBLE;
 import static org.assertj.core.api.InstanceOfAssertFactories.LONG;
 import static org.assertj.core.api.InstanceOfAssertFactories.MAP;
@@ -279,4 +280,11 @@ void shouldMutateWithFilteredNodesAndMultipleProperties() {
         );
     }
 
+    @Test
+    void shouldIllegalArgumentExceptionThrowForNullProperty(){
+        var query = "CALL gds.knn.mutate('myGraph', {nodeProperties: ['foo'],  mutateProperty: 'foo', mutateRelationshipType: 'bar'}) YIELD *";
+        assertThatThrownBy(() -> runQuery(query))
+            .hasMessageContaining("Caused by: java.lang.IllegalArgumentException");
+    }
+
 }