Skip to content

Conversation

Andy2003
Copy link
Collaborator

@Andy2003 Andy2003 commented Jul 3, 2025

This PR refactors the add methods across spatial index classes to use batch operations, deduplicate inputs, and centralize single-node insertion via a default interface method. It also fixes exceptions when deleting nodes attached to multiple layers by properly handling incoming relationships.

  • Introduced add(Transaction, List<Node>) as primary insertion endpoint and removed redundant single-node overrides.
  • Enhanced bulk insertion in RTreeIndex with deduplication via LinkedHashSet and updated rebuild logic.
  • Fixed relationship cleanup in mergeTwoSubtrees and onIndexReference to avoid orphaned or missing relationships.

… and clarity

Fixing an issue where deleting nodes from an index is throwing an exception when node is attached to multiple layers.
@Andy2003 Andy2003 requested a review from Copilot July 3, 2025 10:17
@Andy2003 Andy2003 self-assigned this Jul 3, 2025
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR refactors the add methods across spatial index classes to use batch operations, deduplicate inputs, and centralize single-node insertion via a default interface method. It also fixes exceptions when deleting nodes attached to multiple layers by properly handling incoming relationships.

  • Introduced add(Transaction, List<Node>) as primary insertion endpoint and removed redundant single-node overrides.
  • Enhanced bulk insertion in RTreeIndex with deduplication via LinkedHashSet and updated rebuild logic.
  • Fixed relationship cleanup in mergeTwoSubtrees and onIndexReference to avoid orphaned or missing relationships.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
server-plugin/src/main/java/org/neo4j/gis/spatial/rtree/RTreeIndex.java Refactored add logic for batch insertion, streamlined relationship deletion in tree merges, and updated index‐reference cleanup.
server-plugin/src/main/java/org/neo4j/gis/spatial/index/SpatialIndexWriter.java Added default single-node add implementation that delegates to the batch add method.
server-plugin/src/main/java/org/neo4j/gis/spatial/index/ExplicitIndexBackedPointIndex.java Removed explicit single-node add override and inlined direct index insertions in batch add.
Comments suppressed due to low confidence (3)

server-plugin/src/main/java/org/neo4j/gis/spatial/rtree/RTreeIndex.java:688

  • New logic deletes only relationships whose start node matches the index root. Add a unit test that covers deletion of geometries attached to multiple layers to prevent regression here.
							if (rel.getStartNode().equals(indexRoot)) {

server-plugin/src/main/java/org/neo4j/gis/spatial/rtree/RTreeIndex.java:500

  • [nitpick] The variable name previousParent is a bit misleading since it represents the incoming relationship object. Consider renaming it to existingParentRel or incomingRel for clearer intent.
			Relationship previousParent = n.node.getSingleRelationship(RTreeRelationshipTypes.RTREE_CHILD,

server-plugin/src/main/java/org/neo4j/gis/spatial/rtree/RTreeIndex.java:686

  • The variable indexRoot used inside this block is not in scope, causing a compile error. Consider capturing indexRoot from the invoking method or calling getIndexRoot(tx) here to ensure the correct node is referenced.
					try (var relationships = geomNode.getRelationships(Direction.INCOMING, referenceRelationshipType)) {

@Andy2003 Andy2003 added fix Bugfix and removed bug labels Jul 3, 2025
Copy link

github-actions bot commented Jul 3, 2025

Test Results

343 tests  ±0   318 ✅ ±0   10m 17s ⏱️ -20s
 35 suites ±0    25 💤 ±0 
 35 files   ±0     0 ❌ ±0 

Results for commit 928cd85. ± Comparison against base commit e73f56b.

@Andy2003 Andy2003 merged commit 9964547 into master Jul 3, 2025
2 checks passed
@Andy2003 Andy2003 deleted the bugfix/add-all branch July 3, 2025 10:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant