More types and less null checking

Author: jjaderberg

algo/src/main/java/org/neo4j/gds/similarity/filteredknn/FilteredKnnBaseConfig.java

@@ -28,23 +28,22 @@
 import org.neo4j.gds.similarity.knn.KnnBaseConfig;
 
 import java.util.Collection;
-import java.util.List;
 
 @ValueClass
 @Configuration
 @SuppressWarnings("immutables:subtype")
 public interface FilteredKnnBaseConfig extends KnnBaseConfig {
 
     @Value.Default
-    @Configuration.ConvertWith("org.neo4j.gds.similarity.filteredknn.NodeFilterSpec#create")
+    @Configuration.ConvertWith("org.neo4j.gds.similarity.filteredknn.NodeFilterSpecFactory#create")
     default NodeFilterSpec sourceNodeFilter() {
-        return NodeFilterSpec.create(List.of());
+        return NodeFilterSpec.noOp;
     }
 
     @Value.Default
-    @Configuration.ConvertWith("org.neo4j.gds.similarity.filteredknn.NodeFilterSpec#create")
+    @Configuration.ConvertWith("org.neo4j.gds.similarity.filteredknn.NodeFilterSpecFactory#create")
     default NodeFilterSpec targetNodeFilter() {
-        return NodeFilterSpec.create(List.of());
+        return NodeFilterSpec.noOp;
     }
 
     @Configuration.GraphStoreValidationCheck

algo/src/main/java/org/neo4j/gds/similarity/filteredknn/FilteredKnnResult.java

@@ -100,7 +100,7 @@ public long nodePairsConsidered() {
 
             @Override
             public NodeFilter sourceNodeFilter() {
-                return NodeFilter.noOp();
+                return NodeFilter.noOp;
             }
 
             @Override

algo/src/main/java/org/neo4j/gds/similarity/filteredknn/LabelNodeFilter.java

@@ -0,0 +1,57 @@
+/*
+ * 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.neo4j.gds.NodeLabel;
+import org.neo4j.gds.api.IdMap;
+
+import static org.neo4j.gds.utils.StringFormatting.formatWithLocale;
+
+public final class LabelNodeFilter implements NodeFilter {
+
+    public static LabelNodeFilter create(String labelString, IdMap idMap) {
+        NodeLabel label = null;
+        for (var existingLabel : idMap.availableNodeLabels()) {
+            if (existingLabel.name.equalsIgnoreCase(labelString)) {
+                label = existingLabel;
+            }
+        }
+        if (null == label) {
+            throw new IllegalArgumentException(formatWithLocale(
+                "The label `%s` does not exist in the graph",
+                labelString
+            ));
+        }
+        return new LabelNodeFilter(label, idMap);
+    }
+
+    private final NodeLabel label;
+    private final IdMap idMap;
+
+    private LabelNodeFilter(NodeLabel label, IdMap idMap) {
+        this.label = label;
+        this.idMap = idMap;
+    }
+
+    @Override
+    public boolean test(long nodeId) {
+        return idMap.hasLabel(nodeId, label);
+    }
+}

algo/src/main/java/org/neo4j/gds/similarity/filteredknn/LabelNodeFilterSpec.java

@@ -0,0 +1,36 @@
+/*
+ * 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.neo4j.gds.api.IdMap;
+
+public class LabelNodeFilterSpec implements NodeFilterSpec {
+
+    private final String labelString;
+
+    LabelNodeFilterSpec(String labelString) {
+        this.labelString = labelString;
+    }
+
+    @Override
+    public NodeFilter toNodeFilter(IdMap idMap) {
+        return LabelNodeFilter.create(labelString, idMap);
+    }
+}

algo/src/main/java/org/neo4j/gds/similarity/filteredknn/NodeFilter.java

@@ -19,74 +19,8 @@
  */
 package org.neo4j.gds.similarity.filteredknn;
 
-import org.neo4j.gds.NodeLabel;
-import org.neo4j.gds.api.IdMap;
-
-import java.util.Set;
 import java.util.function.LongPredicate;
-import java.util.stream.Collectors;
-
-import static org.neo4j.gds.utils.StringFormatting.formatWithLocale;
-
-public class NodeFilter implements LongPredicate {
-
-    public static NodeFilter create(Set<Long> externalNodeIds, IdMap idMap) {
-       var mappedNodeIds = externalNodeIds.stream().map(idMap::toMappedNodeId).collect(Collectors.toSet());
-        return new NodeFilter(mappedNodeIds);
-    }
-
-    public static NodeFilter create(String labelString, IdMap idMap) {
-        NodeLabel label = null;
-        for (var existingLabel : idMap.availableNodeLabels()) {
-            if (existingLabel.name.equalsIgnoreCase(labelString)) {
-                label = existingLabel;
-            }
-        }
-        if (null == label) {
-            throw new IllegalArgumentException(formatWithLocale(
-                "The label `%s` does not exist in the graph",
-                labelString
-            ));
-        }
-        return new NodeFilter(label, idMap);
-    }
-
-    public static NodeFilter noOp() {
-        return new NoOpNodeFilter(Set.of());
-    }
-
-    private final Set<Long> nodeIds;
-    private final NodeLabel label;
-    private final IdMap idMap;
-
-    private NodeFilter(Set<Long> nodeIds) {
-        this.nodeIds = nodeIds;
-        this.label = null;
-        this.idMap = null;
-    }
-
-    private NodeFilter(NodeLabel label, IdMap idMap) {
-        this.nodeIds = null;
-        this.label = label;
-        this.idMap = idMap;
-    }
-
-    @Override
-    public boolean test(long nodeId) {
-        return null == nodeIds
-            ? idMap.hasLabel(nodeId, label)
-            : nodeIds.contains(nodeId);
-    }
-
-    private static class NoOpNodeFilter extends NodeFilter {
-
-        NoOpNodeFilter(Set<Long> nodeIds) {
-            super(nodeIds);
-        }
 
-        @Override
-        public boolean test(long nodeId) {
-            return true;
-        }
-    }
+interface NodeFilter extends LongPredicate {
+    NodeFilter noOp = (nodeId) -> true;
 }

algo/src/main/java/org/neo4j/gds/similarity/filteredknn/NodeFilterSpec.java

@@ -20,124 +20,20 @@
 package org.neo4j.gds.similarity.filteredknn;
 
 import org.neo4j.gds.api.IdMap;
-import org.neo4j.graphdb.Node;
-
-import java.util.ArrayList;
-import java.util.HashSet;
-import java.util.List;
-import java.util.Set;
-
-import static org.neo4j.gds.utils.StringFormatting.formatWithLocale;
 
 /**
- * This class serves to do as much parsing and validation as possible in the UI for creating a {@link NodeFilter}.
+ * A {@code NodeFilterSpec} is a partially constructed {@link NodeFilter}. Because we cannot fully construct the
+ * {@link NodeFilter} from user inputs alone, we parse and validate what we can, and prepare for completing the
+ * construction once the rest is available.
  *
- * We can
- * <ul>
- * <li>normalize {@code Long}, {@code List<Long>}, {@code Node} and {@code List<Node>} to {@code Set<Long>}</li>
- * <li>store the normalized {@code Set<Long>}, or the label {@code String}, as the case may be.</li>
- * </ul>
- * But we cannot
- * <ul>
- * <li>validated that the nodes or label exist in the graph.</li>
- * <li>translate node ids from Neo4j id space to the internal id space.</li>
- * </ul>
+ * There are two types of node filters: ones based on a list of node IDs, and ones based on labels.
+ * There are therefore two types of node filter specs, accordingly.
  *
- * The latter two have to happen later, when the {@link NodeFilterSpec} is turned into a {@link NodeFilter}.
+ * The spec is created using {@link NodeFilterSpecFactory#create(Object)} and the {@link NodeFilter} is then created
+ * using {@link NodeFilterSpec#toNodeFilter(org.neo4j.gds.api.IdMap)}.
  */
-public class NodeFilterSpec {
-
-    public static NodeFilterSpec create(Object input) {
-        if (input instanceof NodeFilterSpec) {
-            return (NodeFilterSpec) input;
-        }
-
-        if (input instanceof String) {
-            // parse as label
-            return new NodeFilterSpec((String) input);
-        }
-
-        Set<Long> nodeIds = null;
-
-        if (input instanceof List) {
-            nodeIds = parseFromList((List) input);
-        }
-
-        if (input instanceof Long) {
-            nodeIds = parseFromLong((Long) input);
-        }
-
-        if (input instanceof Node) {
-            nodeIds = parseFromNode((Node) input);
-        }
-
-        if (nodeIds == null) {
-            throw new IllegalArgumentException(
-                formatWithLocale("Invalid scalar type. Expected Long or Node but found: %s", input.getClass().getSimpleName())
-            );
-        }
-
-        return new NodeFilterSpec(nodeIds);
-    }
-
-    private static Set<Long> parseFromLong(Long input) {
-        Set<Long> nodeIds = new HashSet<>();
-        nodeIds.add(input);
-        return nodeIds;
-    }
-
-    private static Set<Long> parseFromNode(Node input) {
-        Set<Long> nodeIds = new HashSet<>();
-        nodeIds.add(input.getId());
-        return nodeIds;
-    }
-
-    private static Set<Long> parseFromList(List input) {
-        Set<Long> nodeIds = new HashSet<>();
-        List<String> badTypes = new ArrayList<>();
-        input.forEach(o -> {
-            if (o instanceof Long) {
-                nodeIds.add((Long) o);
-            } else if (o instanceof Node) {
-                nodeIds.add(((Node) o).getId());
-            } else {
-                badTypes.add(o.getClass().getSimpleName());
-            }
-        });
-
-        if (badTypes.isEmpty()) {
-            return nodeIds;
-        }
-
-        throw new IllegalArgumentException(formatWithLocale(
-            "Invalid types in list. Expected Longs or Nodes but found %s",
-            badTypes
-        ));
-    }
-
-    private final Set<Long> nodeIds;
-    private final String labelString;
-
-    NodeFilterSpec(Set<Long> nodeIds) {
-        this.nodeIds = nodeIds;
-        this.labelString = null;
-    }
-
-    NodeFilterSpec(String labelString) {
-        this.nodeIds = null;
-        this.labelString = labelString;
-    }
+interface NodeFilterSpec {
+    NodeFilter toNodeFilter(IdMap idMap);
 
-    NodeFilter toNodeFilter(IdMap idMap) {
-        if (nodeIds != null) {
-            if (nodeIds.isEmpty()) {
-                return NodeFilter.noOp();
-            }
-            return NodeFilter.create(nodeIds, idMap);
-        }
-        if (labelString != null) {
-            return NodeFilter.create(labelString, idMap);
-        }
-        throw new IllegalStateException("This object is broken. This should not happen, says Jonatan.");
-    }
+    NodeFilterSpec noOp = (idMap) -> NodeFilter.noOp;
 }