DRAFT/WIP: Make rtree package generic over record type #674
Conversation
The `rtree` package previously hardcoded record IDs as integers, requiring users to maintain separate mappings between their records and integer IDs. This added unnecessary indirection and made the API less ergonomic. By making `RTree` generic over the record type T, users can now store their actual records directly in the tree (or use integers if they prefer to keep the existing behaviour). This simplifies usage patterns and eliminates the need for separate index mappings in consuming code. Note: This is a breaking change. The easiest way for users to upgrade is to simply replace all occurrences of `*rtree.RTree` with `*rtree.RTree[int]` in their codebase.
There was a problem hiding this comment.
Pull Request Overview
This PR makes the rtree package generic over record type, eliminating the need for users to maintain separate mappings between integer IDs and their actual records. Previously, the package hardcoded record IDs as integers; now users can store records of any type directly in the tree.
Key changes:
- Made
RTree,node,entry, andBulkItemgeneric over type parameterT - Renamed
RecordIDfield toRecordand changed callback signatures fromfunc(recordID int)tofunc(record T) - Updated all consumers in the
geompackage to useRTree[int]to maintain existing behavior
Reviewed Changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| rtree/rtree.go | Core rtree types made generic with type parameter T |
| rtree/bulk.go | BulkLoad functions updated to support generic record types |
| rtree/nearest.go | Nearest neighbor search functions updated for generic records |
| rtree/box.go | Helper function updated to work with generic nodes |
| rtree/rtree_internal_test.go | Tests updated to use RTree[int] and new Record field |
| rtree/perf_internal_test.go | Performance tests updated for generic API |
| rtree/nearest_internal_test.go | Nearest neighbor tests updated for generic API |
| rtree/golden_internal_test.go | Golden tests updated to reference record field |
| rtree/quick_partition_internal_test.go | Partition tests updated for BulkItem[int] |
| geom/*.go | All rtree consumers updated to use RTree[int] |
| .golangci.yaml | Linter configuration adjusted for generic types |
| CHANGELOG.md | Breaking change documented with migration guidance |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| errcheck: | ||
| exclude-functions: | ||
| - io.Copy(os.Stdout) | ||
| - (*github.com/peterstace/simplefeatures/rtree.RTree).RangeSearch |
There was a problem hiding this comment.
errcheck seems to get a bit confused when generics are introduced... I can longer specify funcs in exclude-functions section. Instead, I've opted to just use _ = ... whenever I want to ignore an error.
| - gomnd | ||
| - inamedparam | ||
| - interfacebloat | ||
| - ireturn |
There was a problem hiding this comment.
I return has a false positive when using generics. Seems like not really that useful for this project, so disabled.
There was a problem hiding this comment.
TODOs:
- Run benchmarks tests to check for a performance regression.
- Update
geompackage to make better use of the generic RTree (we shouldn't need the separate arrays to hold the records any more!).
peterstace
changed the title
Description
The
rtreepackage previously hardcoded record IDs as integers, requiring users to maintain separate mappings between their records and integer IDs. This added unnecessary indirection and made the API less ergonomic.By making
RTreegeneric over the record type T, users can now store their actual records directly in the tree (or use integers if they prefer to keep the existing behaviour). This simplifies usage patterns and eliminates the need for separate index mappings in consuming code.Note: This is a breaking change. The easiest way for users to upgrade is to simply replace all occurrences of
*rtree.RTreewith*rtree.RTree[int]in their codebase.Check List
Have you:
Added unit tests? Yes.
Add cmprefimpl tests? (if appropriate?) N/A
Updated release notes? (if appropriate?) Yes.