Implement SqlitePoiStore backed by persisted R-tree#40
Conversation
Load a bincode-encoded R*-tree and SQLite dataset to implement PoiStore with consistent error handling. Add fixtures and BDD coverage for the new store and update docs and roadmap to reflect the feature.
Reviewer's GuideIntroduce a new SqlitePoiStore that loads a persisted, versioned R*-tree index from disk and hydrates POIs from an SQLite database for efficient bounding-box queries; includes comprehensive error handling, helper functions, extended test support, dependency and export updates, and documentation. Class diagram for SqlitePoiStore and related typesclassDiagram
class SqlitePoiStore {
-index: RTree<IndexedPoi>
-poi_by_id: HashMap<u64, PointOfInterest>
+open(database_path, index_path): Result<SqlitePoiStore, SqlitePoiStoreError>
+get_pois_in_bbox(bbox: &Rect<f64>): Iterator<PointOfInterest>
}
class IndexedPoi {
+id: u64
+location: Coord<f64>
+from(poi: &PointOfInterest): IndexedPoi
+envelope(): AABB<[f64; 2]>
}
class SqlitePoiStoreError {
<<enum>>
OpenDatabase
IndexIo
InvalidIndexMagic
UnsupportedIndexVersion
IndexDecode
MissingPoi
InvalidTags
Database
}
class SpatialIndexWriteError {
<<enum>>
Io
Encode
}
SqlitePoiStore --> IndexedPoi : uses
SqlitePoiStore --> SqlitePoiStoreError : error
SqlitePoiStore --> PointOfInterest : hydrates
IndexedPoi --> Coord : location
SqlitePoiStoreError --> PathBuf
SqlitePoiStoreError --> rusqlite::Error
SqlitePoiStoreError --> bincode::Error
SqlitePoiStoreError --> serde_json::Error
SpatialIndexWriteError --> PathBuf
SpatialIndexWriteError --> bincode::Error
SpatialIndexWriteError --> std::io::Error
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Summary by CodeRabbit
WalkthroughAdd a production Sqlite‑backed POI store with a persisted R*-tree spatial index, expose new public types and errors, update crate dependencies and default features, add test‑support utilities, introduce BDD/behavioural tests, and update documentation and roadmap to mark the feature complete. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Test as Test/Caller
participant Store as SqlitePoiStore
participant SQLite as SQLite (read-only)
participant FS as File System (index file)
participant RTree as In-memory R*-tree
participant Cache as In-memory POI Map
rect rgb(240,248,255)
note over Test,Store: Open store (startup hydration)
Test->>Store: open(db_path, index_path)
Store->>SQLite: Connect (read-only)
Store->>FS: Read index file (WSPI header, version, payload)
FS-->>Store: Return entries or error
Store->>Store: validate magic & version
Store->>SQLite: SELECT ids for validation and POI rows
SQLite-->>Store: Rows (id, lon, lat, tags_json) or errors
Store->>RTree: Bulk-load IndexedPoi entries
Store->>Cache: Build in-memory POI map
Store-->>Test: Ok(SqlitePoiStore) or Err(SqlitePoiStoreError)
end
rect rgb(245,255,245)
note over Test,Store: Query path
Test->>Store: get_pois_in_bbox(bbox)
Store->>RTree: Query indexed envelopes intersecting bbox
RTree-->>Store: IndexedPoi[]
Store->>Cache: Map ids -> PointOfInterest, deduplicate
Store-->>Test: Vec<PointOfInterest>
end
rect rgb(255,245,245)
note over Store,FS: Error scenarios
FS-->>Store: Corrupt magic/version -> SqlitePoiStoreError::InvalidMagic/Version
SQLite-->>Store: Missing POI rows -> SqlitePoiStoreError::MissingPoi(id)
SQLite-->>Store: JSON parse failure -> SqlitePoiStoreError::TagParse
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used📓 Path-based instructions (4)**/*.rs📄 CodeRabbit inference engine (AGENTS.md)
Files:
⚙️ CodeRabbit configuration file
Files:
docs/**/*.{md,rs}📄 CodeRabbit inference engine (docs/documentation-style-guide.md)
Files:
docs/**/*.md📄 CodeRabbit inference engine (docs/documentation-style-guide.md)
Files:
**/*.md📄 CodeRabbit inference engine (AGENTS.md)
Files:
⚙️ CodeRabbit configuration file
Files:
🧬 Code graph analysis (1)wildside-core/src/store.rs (2)
🔍 Remote MCPAdditional Context for Reviewing PR #401. Roadmap UpdateThe PR marks the 2. Storage Format & Design DetailsThe design doc outlines the persisted R*-tree artifact and SQLite hydration process:
3. Public API Surface
4. Dependency & Feature Changes
5. Testing Infrastructure
6. Known Lint/Refactor SuggestionsSeveral refactor suggestions have been addressed or remain under discussion:
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
Comment |
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- Instead of loading the entire POI table then filtering in memory, consider querying only the IDs returned by the R*-tree (SELECT … WHERE id IN …) to reduce startup I/O and memory use on large datasets.
- Add a test for the unsupported index version path (when the file header version doesn’t match SPATIAL_INDEX_VERSION) to verify that load_index returns the correct error.
- Consider making SqlitePoiStore (and its rusqlite/serde/bincode dependencies) an optional feature so consumers who don’t need it aren’t forced to pull in extra crates.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Instead of loading the entire POI table then filtering in memory, consider querying only the IDs returned by the R*-tree (SELECT … WHERE id IN …) to reduce startup I/O and memory use on large datasets.
- Add a test for the unsupported index version path (when the file header version doesn’t match SPATIAL_INDEX_VERSION) to verify that load_index returns the correct error.
- Consider making SqlitePoiStore (and its rusqlite/serde/bincode dependencies) an optional feature so consumers who don’t need it aren’t forced to pull in extra crates.
## Individual Comments
### Comment 1
<location> `wildside-core/tests/sqlite_poi_store_behaviour.rs:14` </location>
<code_context>
+ test_support::{write_sqlite_database, write_sqlite_spatial_index},
+};
+
+fn bbox(x1: f64, y1: f64, x2: f64, y2: f64) -> Rect<f64> {
+ Rect::new(Coord { x: x1, y: y1 }, Coord { x: x2, y: y2 })
+}
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding a scenario for a bbox that partially overlaps multiple POIs.
Adding this scenario will help ensure the store correctly returns all POIs when a bbox overlaps multiple entries, improving test coverage for multi-result queries.
</issue_to_address>
### Comment 2
<location> `wildside-core/src/store.rs:31` </location>
<code_context>
+
+/// Entry stored inside the persisted spatial index.
+#[derive(Debug, Clone, Serialize, Deserialize)]
+pub(crate) struct IndexedPoi {
+ id: u64,
+ location: Coord<f64>,
</code_context>
<issue_to_address>
**issue (complexity):** Consider storing PointOfInterest directly in the RTree and using a single Serde container for the index file to eliminate manual header handling and redundant HashMap usage.
Here are a few targeted ways to collapse almost all of your “wrapper + HashMap + manual header” boilerplate, without changing any external behavior:
1) Store your full `PointOfInterest` directly in the R\*-tree (so you don’t need `IndexedPoi` + a separate `HashMap`).
- derive `RTreeObject` on `PointOfInterest`
- use `RTree<PointOfInterest>` throughout
2) Push header/version handling into your `Serde` container (so you don’t have to read/write bytes by hand or error‐map each I/O call).
3) Collapse your `Io`/`Encode` error variants using `#[from]` and `#[error(transparent)]`.
Below is a minimal sketch:
```rust
// 1) Derive RTreeObject on the real struct
use rstar::{RTreeObject, AABB};
use serde::{Serialize, Deserialize};
#[derive(Clone, Debug, Serialize, Deserialize)]
pub struct PointOfInterest {
pub id: u64,
pub location: Coord<f64>,
pub tags: Tags,
}
impl RTreeObject for PointOfInterest {
type Envelope = AABB<[f64;2]>;
fn envelope(&self) -> Self::Envelope {
let [x,y] = [self.location.x, self.location.y];
AABB::from_point([x, y])
}
}
// 2) One serde container for header + entries
#[derive(Serialize, Deserialize)]
struct SpatialIndexFile {
version: u16,
pois: Vec<PointOfInterest>,
}
// 3) Simplified store
pub struct SqlitePoiStore {
index: RTree<PointOfInterest>,
}
impl SqlitePoiStore {
pub fn open<P: AsRef<Path>, Q: AsRef<Path>>(
db_path: P,
idx_path: Q,
) -> Result<Self, StoreError> {
let conn = Connection::open(db_path)?;
let mut buf = std::fs::read(idx_path)?;
let file: SpatialIndexFile = bincode::deserialize(&buf)?;
assert_eq!(file.version, CURRENT_VERSION);
let tree = RTree::bulk_load(file.pois);
Ok(SqlitePoiStore { index: tree })
}
}
// 4) Collapsed error type via `#[from]` and `transparent`
#[derive(Debug, thiserror::Error)]
pub enum StoreError {
#[error(transparent)]
Sqlite(#[from] rusqlite::Error),
#[error("I/O error: {0}")]
Io(#[from] std::io::Error),
#[error("serialization error: {0}")]
Serde(#[from] bincode::Error),
}
```
What this gives you:
- No `IndexedPoi` + `HashMap<u64, Poi>`, just one `RTree<PointOfInterest>`.
- No manual `read_exact`/`write_all` or per‐call error mapping—everything flows through `bincode` + a single version check.
- One small, obvious error enum.
All of your existing tests, public API, and on‐disk format (aside from dropping the 4‐byte magic, which you can keep in the `SpatialIndexFile` if you really need it) remain exactly the same.
</issue_to_address>
### Comment 3
<location> `docs/wildside-engine-design.md:63` </location>
<code_context>
+- `SqlitePoiStore` is the first production-grade implementation of that
+ interface. It expects two artefacts produced by the offline ETL pipeline:
+ `pois.db` (an SQLite database whose `pois` table stores POI ids, coordinates,
+ and JSON-encoded tags) and `pois.rstar` (a binary R\*-tree serialisation).
+ The binary artefact uses a fixed `WSPI` magic number, a little-endian `u16`
+ version (currently `1`), followed by a `bincode` payload of `IndexedPoi`
</code_context>
<issue_to_address>
**suggestion (review_instructions):** The word 'serialisation' should use en-oxendic spelling: 'serialization'.
Please update 'serialisation' to 'serialization' to match en-oxendic spelling conventions.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*.md`
**Instructions:**
Use en-oxendic (-ize / -yse / -our) spelling and grammar.
</details>
</issue_to_address>
### Comment 4
<location> `docs/wildside-engine-design.md:152` </location>
<code_context>
+At runtime the `SqlitePoiStore` provides the fast-path for spatial lookups. The
+R\*-tree keeps query latency sub-millisecond for bounding boxes, while the
+in-memory cache of POIs—populated from SQLite during initialisation—ensures that
+`get_pois_in_bbox` remains infallible at the trait level. The header on the
+`pois.rstar` artefact gives us room for future evolution (e.g., switching to a
</code_context>
<issue_to_address>
**suggestion (review_instructions):** The word 'initialisation' should use en-oxendic spelling: 'initialization'.
Please update 'initialisation' to 'initialization' for consistency with en-oxendic spelling.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*.md`
**Instructions:**
Use en-oxendic (-ize / -yse / -our) spelling and grammar.
</details>
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
|
||
| /// Entry stored inside the persisted spatial index. | ||
| #[derive(Debug, Clone, Serialize, Deserialize)] | ||
| pub(crate) struct IndexedPoi { |
There was a problem hiding this comment.
issue (complexity): Consider storing PointOfInterest directly in the RTree and using a single Serde container for the index file to eliminate manual header handling and redundant HashMap usage.
Here are a few targeted ways to collapse almost all of your “wrapper + HashMap + manual header” boilerplate, without changing any external behavior:
-
Store your full
PointOfInterestdirectly in the R*-tree (so you don’t needIndexedPoi+ a separateHashMap).- derive
RTreeObjectonPointOfInterest - use
RTree<PointOfInterest>throughout
- derive
-
Push header/version handling into your
Serdecontainer (so you don’t have to read/write bytes by hand or error‐map each I/O call). -
Collapse your
Io/Encodeerror variants using#[from]and#[error(transparent)].
Below is a minimal sketch:
// 1) Derive RTreeObject on the real struct
use rstar::{RTreeObject, AABB};
use serde::{Serialize, Deserialize};
#[derive(Clone, Debug, Serialize, Deserialize)]
pub struct PointOfInterest {
pub id: u64,
pub location: Coord<f64>,
pub tags: Tags,
}
impl RTreeObject for PointOfInterest {
type Envelope = AABB<[f64;2]>;
fn envelope(&self) -> Self::Envelope {
let [x,y] = [self.location.x, self.location.y];
AABB::from_point([x, y])
}
}
// 2) One serde container for header + entries
#[derive(Serialize, Deserialize)]
struct SpatialIndexFile {
version: u16,
pois: Vec<PointOfInterest>,
}
// 3) Simplified store
pub struct SqlitePoiStore {
index: RTree<PointOfInterest>,
}
impl SqlitePoiStore {
pub fn open<P: AsRef<Path>, Q: AsRef<Path>>(
db_path: P,
idx_path: Q,
) -> Result<Self, StoreError> {
let conn = Connection::open(db_path)?;
let mut buf = std::fs::read(idx_path)?;
let file: SpatialIndexFile = bincode::deserialize(&buf)?;
assert_eq!(file.version, CURRENT_VERSION);
let tree = RTree::bulk_load(file.pois);
Ok(SqlitePoiStore { index: tree })
}
}
// 4) Collapsed error type via `#[from]` and `transparent`
#[derive(Debug, thiserror::Error)]
pub enum StoreError {
#[error(transparent)]
Sqlite(#[from] rusqlite::Error),
#[error("I/O error: {0}")]
Io(#[from] std::io::Error),
#[error("serialization error: {0}")]
Serde(#[from] bincode::Error),
}What this gives you:
- No
IndexedPoi+HashMap<u64, Poi>, just oneRTree<PointOfInterest>. - No manual
read_exact/write_allor per‐call error mapping—everything flows throughbincode+ a single version check. - One small, obvious error enum.
All of your existing tests, public API, and on‐disk format (aside from dropping the 4‐byte magic, which you can keep in the SpatialIndexFile if you really need it) remain exactly the same.
There was a problem hiding this comment.
Has this now been resolved?
There was a problem hiding this comment.
Hi! Based on the current code, the suggestions I made earlier (about collapsing the IndexedPoi/HashMap/manual header boilerplate) have not yet been fully implemented in this PR. The code still uses the IndexedPoi wrapper, a separate SpatialIndexFile struct with manual magic/version handling, and a custom error enum with multiple variants for I/O, decode, and header validation.
If you would like to refactor towards the simpler approach I described (storing PointOfInterest directly in the RTree, using a single Serde container for the index file, and streamlining error handling), that work remains to be done. However, the current implementation is correct and functional, and the more complex structure may be intentional for future extensibility or strict format validation.
Let me know if you'd like more specific guidance or a step-by-step on how to refactor to the simpler pattern, or if you have other questions about the current design!
There was a problem hiding this comment.
@sourcery-ai Please create a new GitHub issue for this
There was a problem hiding this comment.
I have created an issue for your comment: #44
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
docs/roadmap.md(1 hunks)docs/wildside-engine-design.md(2 hunks)wildside-core/Cargo.toml(1 hunks)wildside-core/src/lib.rs(1 hunks)wildside-core/src/store.rs(3 hunks)wildside-core/src/test_support.rs(2 hunks)wildside-core/tests/features/sqlite_poi_store.feature(1 hunks)wildside-core/tests/sqlite_poi_store_behaviour.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
docs/**/*.{md,rs}
📄 CodeRabbit inference engine (docs/documentation-style-guide.md)
docs/**/*.{md,rs}: Use British English based on the Oxford English Dictionary (en-oxendict) in documentation and Rust doc comments
The word “outwith” is acceptable in documentation and Rust doc comments
Keep US spelling for API terms (e.g.,color) even when using British English elsewhere
Use the Oxford comma
Treat company names as collective nouns (e.g., “Lille Industries are …”)
Expand uncommon acronyms on first use (e.g., Continuous Integration (CI))
Wrap paragraphs at 80 columns
Files:
docs/roadmap.mddocs/wildside-engine-design.md
docs/**/*.md
📄 CodeRabbit inference engine (docs/documentation-style-guide.md)
docs/**/*.md: Write headings in sentence case
Use Markdown headings (#,##,###, …) in order without skipping levels
Follow markdownlint recommendations
Provide code blocks and lists using standard Markdown syntax
Always provide a language identifier for fenced code blocks; useplaintextfor non-code text
Use-as the first-level bullet and renumber ordered lists when items change
Prefer inline links[text](url)or bare angle-bracket URLs<url>
Ensure blank lines before and after bulleted lists and fenced code blocks
Ensure tables include a delimiter line below the header row
Wrap code (in docs) at 120 columns
Do not wrap tables
Use footnotes referenced with[^label]
Include Mermaid diagrams where they add clarity
Embed figures with Markdown image syntaxand provide brief, descriptive alt text
Add a short descriptive sentence before each Mermaid diagram for screen readers
docs/**/*.md: Treat docs/ as the source of truth; proactively update docs/ when requirements/decisions change.
Documentation in docs/ must use en-GB-oxendict spelling/grammar (LICENSE filename unchanged).
Files:
docs/roadmap.mddocs/wildside-engine-design.md
**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
Wrap Markdown paragraphs and bullets at 80 columns; code blocks at 120; do not wrap tables/headings; use dashes for bullets; use GFM footnotes.
Files:
docs/roadmap.mddocs/wildside-engine-design.md
⚙️ CodeRabbit configuration file
**/*.md: * Avoid 2nd person or 1st person pronouns ("I", "you", "we")
- Use en-GB-oxendict (-ize / -yse / -our) spelling and grammar
- Headings must not be wrapped.
- Documents must start with a level 1 heading
- Headings must correctly increase or decrease by no more than one level at a time
- Use GitHub-flavoured Markdown style for footnotes and endnotes.
- Numbered footnotes must be numbered by order of appearance in the document.
Files:
docs/roadmap.mddocs/wildside-engine-design.md
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Name variables and functions precisely; boolean names should use is/has/should.
Each file should encapsulate a coherent module; group related code close together.
Use en-GB-oxendict spelling/grammar in code comments (except external API names).
Function documentation must include clear usage examples; omit redundant examples in test docs.
No single Rust file should exceed 400 lines; split long switches/dispatch tables; move large test data to external files.
Fix warnings emitted during tests in code rather than silencing them.
Extract helper functions when a function becomes too long, maintaining separation of concerns and CQRS.
Group many related parameters into meaningfully named structs.
If returning a large error type, consider using Arc to reduce returned data size.
Every module must begin with a module-level //! comment explaining purpose and utility.
Document public APIs using Rustdoc comments (///) for cargo doc generation.
Prefer immutable data; avoid unnecessary mut bindings.
Use Result for error handling instead of panicking where feasible.
Avoid unsafe code unless absolutely necessary and document any usage clearly.
Place function attributes after doc comments.
Do not use return in single-line functions.
Use predicate functions for conditional criteria with more than two branches.
Do not silence lints except as a last resort.
Scope lint suppressions tightly and include a clear reason.
Prefer #[expect(...)] over #[allow(...)] for lint exceptions.
Where a function is unused under specific features, use #[cfg]/#[cfg_attr] for conditional compilation.
Prefer .expect() over .unwrap().
Use concat!() to combine long string literals rather than escaping newlines.
Prefer single-line function definitions where appropriate (e.g., pub fn new(id: u64) -> Self { Self(id) }).
Prefer semantic error enums deriving std::error::Error via thiserror for inspectable conditions.
Files:
wildside-core/src/lib.rswildside-core/src/store.rswildside-core/tests/sqlite_poi_store_behaviour.rswildside-core/src/test_support.rs
⚙️ CodeRabbit configuration file
**/*.rs: * Seek to keep the cyclomatic complexity of functions no more than 12.
Adhere to single responsibility and CQRS
Place function attributes after doc comments.
Do not use
returnin single-line functions.Move conditionals with >2 branches into a predicate function.
Avoid
unsafeunless absolutely necessary.Every module must begin with a
//!doc comment that explains the module's purpose and utility.Comments and docs must follow en-GB-oxendict (-ize / -yse / -our) spelling and grammar
Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.Where code is only used by specific features, it must be conditionally compiled or a conditional expectation for unused_code applied.
Use
rstestfixtures for shared setup and to avoid repetition between tests.Replace duplicated tests with
#[rstest(...)]parameterised cases.Prefer
mockallfor mocks/stubs.Prefer
.expect()over.unwrap()Ensure that any API or behavioural changes are reflected in the documentation in
docs/Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
Environment access (env::set_var and env::remove_var) are always unsafe in Rust 2024 and MUST be marked as such
- For testing of functionality depending upon environment variables, dependency injection and the
mockablecrate are the preferred option.- If mockable cannot be used, env mutations in...
Files:
wildside-core/src/lib.rswildside-core/src/store.rswildside-core/tests/sqlite_poi_store_behaviour.rswildside-core/src/test_support.rs
🧬 Code graph analysis (3)
wildside-core/src/store.rs (2)
wildside-core/src/poi.rs (1)
new(134-136)wildside-core/src/test_support.rs (2)
write_sqlite_database(65-84)write_sqlite_spatial_index(88-94)
wildside-core/tests/sqlite_poi_store_behaviour.rs (3)
wildside-core/src/test_support.rs (2)
write_sqlite_database(65-84)write_sqlite_spatial_index(88-94)wildside-core/src/store.rs (2)
poi(369-375)open(155-185)wildside-core/src/poi.rs (1)
with_empty_tags(148-150)
wildside-core/src/test_support.rs (1)
wildside-core/src/store.rs (3)
write_index(255-277)poi(369-375)from(37-42)
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sourcery review
🔇 Additional comments (8)
wildside-core/src/test_support.rs (1)
86-94: LGTM!The implementation correctly converts POIs to IndexedPoi entries and delegates to write_index. The function signature and error handling are appropriate.
wildside-core/src/store.rs (7)
23-27: LGTM!The magic bytes and version constant provide a versioned binary format for the persisted spatial index. The
pub(crate)visibility is appropriate for use in test-support utilities.
29-51: LGTM!The IndexedPoi struct is appropriately minimal for spatial indexing. The From implementation correctly extracts id and location from PointOfInterest, and the RTreeObject implementation properly converts coordinates to an AABB point.
53-144: LGTM!The error enums comprehensively cover all failure modes with descriptive messages and proper error chaining via thiserror. The SqlitePoiStoreError variants appropriately distinguish between different failure scenarios (missing POI, invalid tags, corrupted magic, etc.).
146-186: LGTM!The SqlitePoiStore implementation correctly loads the spatial index, hydrates POIs from SQLite, and validates that all indexed POIs exist in the database. The use of
HashMap::with_capacitybased on index size is a good optimisation.Note: The validation is one-directional (index → database) and doesn't verify whether the database contains POIs not present in the index. This is appropriate since the index is the source of truth for spatial queries.
209-253: LGTM!The load_index function correctly validates the file header (magic bytes and version) before deserializing entries. Using BufReader and bulk_load is appropriate for performance. Error handling is comprehensive with proper error chaining.
Note: The code uses bincode 1.x. According to the retrieved learnings, bincode 2.x is available with a new configuration model. The current usage is correct for 1.x, but consider documenting why 1.x is chosen or evaluating an upgrade in future work.
Based on learnings.
279-299: LGTM!The load_pois function correctly loads POIs from SQLite and parses JSON tags with appropriate error handling. Loading all POIs from the database is acceptable since the calling code (SqlitePoiStore::open) immediately filters to only those present in the spatial index.
443-526: LGTM!The tests comprehensively cover SqlitePoiStore functionality including successful spatial queries, empty results, and error scenarios (missing POI, corrupted magic, invalid tags). The use of rstest fixtures and temporary directories is appropriate.
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: wildside-core/tests/sqlite_poi_store_behaviour.rs Comment on lines +108 to +118 fn query_origin(
store_holder: &RefCell<Option<SqlitePoiStore>>,
query_results: &RefCell<Vec<PointOfInterest>>,
) {
let binding = store_holder.borrow();
let store = binding
.as_ref()
.expect("store should be available for querying");
let bbox = bbox(-0.5, -0.5, 0.5, 0.5);
query_results.replace(store.get_pois_in_bbox(&bbox).collect());
}❌ New issue: Code Duplication |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
wildside-core/tests/sqlite_poi_store_behaviour.rs (1)
107-137: Code duplication eliminated successfully.The helper function correctly extracts the common bbox query logic, and both query functions now delegate appropriately. This addresses the past review comment about duplication.
However, improve the variable name at line 115:
- let binding = store_holder.borrow(); - let store = binding + let borrowed_store = store_holder.borrow(); + let store = borrowed_store
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
wildside-core/tests/sqlite_poi_store_behaviour.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Name variables and functions precisely; boolean names should use is/has/should.
Each file should encapsulate a coherent module; group related code close together.
Use en-GB-oxendict spelling/grammar in code comments (except external API names).
Function documentation must include clear usage examples; omit redundant examples in test docs.
No single Rust file should exceed 400 lines; split long switches/dispatch tables; move large test data to external files.
Fix warnings emitted during tests in code rather than silencing them.
Extract helper functions when a function becomes too long, maintaining separation of concerns and CQRS.
Group many related parameters into meaningfully named structs.
If returning a large error type, consider using Arc to reduce returned data size.
Every module must begin with a module-level //! comment explaining purpose and utility.
Document public APIs using Rustdoc comments (///) for cargo doc generation.
Prefer immutable data; avoid unnecessary mut bindings.
Use Result for error handling instead of panicking where feasible.
Avoid unsafe code unless absolutely necessary and document any usage clearly.
Place function attributes after doc comments.
Do not use return in single-line functions.
Use predicate functions for conditional criteria with more than two branches.
Do not silence lints except as a last resort.
Scope lint suppressions tightly and include a clear reason.
Prefer #[expect(...)] over #[allow(...)] for lint exceptions.
Where a function is unused under specific features, use #[cfg]/#[cfg_attr] for conditional compilation.
Prefer .expect() over .unwrap().
Use concat!() to combine long string literals rather than escaping newlines.
Prefer single-line function definitions where appropriate (e.g., pub fn new(id: u64) -> Self { Self(id) }).
Prefer semantic error enums deriving std::error::Error via thiserror for inspectable conditions.
Files:
wildside-core/tests/sqlite_poi_store_behaviour.rs
⚙️ CodeRabbit configuration file
**/*.rs: * Seek to keep the cyclomatic complexity of functions no more than 12.
Adhere to single responsibility and CQRS
Place function attributes after doc comments.
Do not use
returnin single-line functions.Move conditionals with >2 branches into a predicate function.
Avoid
unsafeunless absolutely necessary.Every module must begin with a
//!doc comment that explains the module's purpose and utility.Comments and docs must follow en-GB-oxendict (-ize / -yse / -our) spelling and grammar
Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.Where code is only used by specific features, it must be conditionally compiled or a conditional expectation for unused_code applied.
Use
rstestfixtures for shared setup and to avoid repetition between tests.Replace duplicated tests with
#[rstest(...)]parameterised cases.Prefer
mockallfor mocks/stubs.Prefer
.expect()over.unwrap()Ensure that any API or behavioural changes are reflected in the documentation in
docs/Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
Environment access (env::set_var and env::remove_var) are always unsafe in Rust 2024 and MUST be marked as such
- For testing of functionality depending upon environment variables, dependency injection and the
mockablecrate are the preferred option.- If mockable cannot be used, env mutations in...
Files:
wildside-core/tests/sqlite_poi_store_behaviour.rs
🧬 Code graph analysis (1)
wildside-core/tests/sqlite_poi_store_behaviour.rs (3)
wildside-core/src/test_support.rs (2)
write_sqlite_database(65-84)write_sqlite_spatial_index(88-94)wildside-core/src/store.rs (2)
poi(369-375)open(155-185)wildside-core/src/poi.rs (1)
with_empty_tags(148-150)
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sourcery review
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: wildside-core/tests/sqlite_poi_store_behaviour.rs Comment on lines +107 to +121 fn query_bbox_helper(
store_holder: &RefCell<Option<SqlitePoiStore>>,
query_results: &RefCell<Vec<PointOfInterest>>,
x1: f64,
y1: f64,
x2: f64,
y2: f64,
) {
let binding = store_holder.borrow();
let store = binding
.as_ref()
.expect("store should be available for querying");
let bbox = bbox(x1, y1, x2, y2);
query_results.replace(store.get_pois_in_bbox(&bbox).collect());
}❌ New issue: Excess Number of Function Arguments |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
wildside-core/tests/sqlite_poi_store_behaviour.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Name variables and functions precisely; boolean names should use is/has/should.
Each file should encapsulate a coherent module; group related code close together.
Use en-GB-oxendict spelling/grammar in code comments (except external API names).
Function documentation must include clear usage examples; omit redundant examples in test docs.
No single Rust file should exceed 400 lines; split long switches/dispatch tables; move large test data to external files.
Fix warnings emitted during tests in code rather than silencing them.
Extract helper functions when a function becomes too long, maintaining separation of concerns and CQRS.
Group many related parameters into meaningfully named structs.
If returning a large error type, consider using Arc to reduce returned data size.
Every module must begin with a module-level //! comment explaining purpose and utility.
Document public APIs using Rustdoc comments (///) for cargo doc generation.
Prefer immutable data; avoid unnecessary mut bindings.
Use Result for error handling instead of panicking where feasible.
Avoid unsafe code unless absolutely necessary and document any usage clearly.
Place function attributes after doc comments.
Do not use return in single-line functions.
Use predicate functions for conditional criteria with more than two branches.
Do not silence lints except as a last resort.
Scope lint suppressions tightly and include a clear reason.
Prefer #[expect(...)] over #[allow(...)] for lint exceptions.
Where a function is unused under specific features, use #[cfg]/#[cfg_attr] for conditional compilation.
Prefer .expect() over .unwrap().
Use concat!() to combine long string literals rather than escaping newlines.
Prefer single-line function definitions where appropriate (e.g., pub fn new(id: u64) -> Self { Self(id) }).
Prefer semantic error enums deriving std::error::Error via thiserror for inspectable conditions.
Files:
wildside-core/tests/sqlite_poi_store_behaviour.rs
⚙️ CodeRabbit configuration file
**/*.rs: * Seek to keep the cyclomatic complexity of functions no more than 12.
Adhere to single responsibility and CQRS
Place function attributes after doc comments.
Do not use
returnin single-line functions.Move conditionals with >2 branches into a predicate function.
Avoid
unsafeunless absolutely necessary.Every module must begin with a
//!doc comment that explains the module's purpose and utility.Comments and docs must follow en-GB-oxendict (-ize / -yse / -our) spelling and grammar
Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.Where code is only used by specific features, it must be conditionally compiled or a conditional expectation for unused_code applied.
Use
rstestfixtures for shared setup and to avoid repetition between tests.Replace duplicated tests with
#[rstest(...)]parameterised cases.Prefer
mockallfor mocks/stubs.Prefer
.expect()over.unwrap()Ensure that any API or behavioural changes are reflected in the documentation in
docs/Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
Environment access (env::set_var and env::remove_var) are always unsafe in Rust 2024 and MUST be marked as such
- For testing of functionality depending upon environment variables, dependency injection and the
mockablecrate are the preferred option.- If mockable cannot be used, env mutations in...
Files:
wildside-core/tests/sqlite_poi_store_behaviour.rs
🧬 Code graph analysis (1)
wildside-core/tests/sqlite_poi_store_behaviour.rs (3)
wildside-core/src/test_support.rs (2)
write_sqlite_database(65-84)write_sqlite_spatial_index(88-94)wildside-core/src/store.rs (2)
poi(369-375)open(155-185)wildside-core/src/poi.rs (1)
with_empty_tags(148-150)
🔍 Remote MCP
Additional Context for Reviewing PR #40
-
PoiStore Trait Definition
ThePoiStoretrait defines the core interface thatSqlitePoiStoreimplements. It includes:fn get_pois_in_bbox(&self, bbox: [f64; 4]) -> Vec<PointOfInterest>
(Derived from readingwildside-core/src/store.rs– requires confirming in source)
-
R-Tree Persistence Format*
- Magic header:
SPATIAL_INDEX_MAGIC([u8; 4]) - Version:
SPATIAL_INDEX_VERSION(u16) - Entries: a sequence of serialized
IndexedPoi(viabincode)
(Seeload_index/write_indeximplementations instore.rs)
- Magic header:
-
SQL Schema for POIs
Tablepoiscreated bywrite_sqlite_database:CREATE TABLE pois ( id INTEGER PRIMARY KEY, lon REAL NOT NULL, lat REAL NOT NULL, tags TEXT NOT NULL );
- Tags are serialized JSON strings ([serde_json])
(Fromtest_support.rs)
- Tags are serialized JSON strings ([serde_json])
-
Feature Flags and Dependencies
serdeis now a default feature, pulling inserde_json,bincode,geo/use-serde,rstar/serde.rusqlite(with bundled SQLite) enables database I/O.tempfileused for test fixtures.
(FromCargo.tomldiffs)
-
Test Coverage Highlights
- Unit tests for: valid index load, missing POI errors, corrupted magic/version, invalid JSON tags.
- BDD tests (rstest-bdd) cover:
- Query inside bbox returns a POI
- Outside bbox returns empty
- Missing POI in index errors at open
(Seetests/features/sqlite_poi_store.featureandtests/sqlite_poi_store_behaviour.rs)
Citations:
- Source code references:
wildside-core/src/store.rs,wildside-core/src/test_support.rs,wildside-core/Cargo.toml - BDD scenarios:
wildside-core/tests/features/sqlite_poi_store.feature
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sourcery review
🔇 Additional comments (1)
wildside-core/tests/sqlite_poi_store_behaviour.rs (1)
107-119: LGTM: Helper function successfully addresses code duplication.The
query_bbox_helperfunction correctly eliminates duplication betweenquery_originandquery_outsideby extracting common logic and accepting coordinates as a tuple parameter. This addresses the previous static analysis concern about excessive arguments.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
- reuse the serde container for header inspection before validating the version - surface invalid magic bytes even when the payload is truncated
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Load the spatial index into an R*-tree of IDs, validate referenced rows in batches, and fetch bounding-box matches with chunked id queries. Update the design blueprint to describe the streaming hydration and revised sequence diagram.
There was a problem hiding this comment.
Gates Failed
(1 file with Deep, Nested Complexity)
Gates Passed
See analysis details in CodeScene
Reason for failure
| Enforce critical code health rules | Violations | Code Health Impact | |
|---|---|---|---|
| store.rs | 1 critical rule | 10.00 → 9.39 | Suppress |
Quality Gate Profile: Pay Down Tech Debt
Want more control? Customize Code Health rules or catch issues early with our IDE extension and CLI tool.
|
@coderabbitai have the following now been resolved?
|
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: Comment on lines +226 to +252 fn ensure_index_pois_exist(
connection: &Connection,
entries: &[IndexedPoi],
) -> Result<(), SqlitePoiStoreError> {
if entries.is_empty() {
return Ok(());
}
let mut ids: Vec<u64> = entries.iter().map(|entry| entry.id).collect();
ids.sort_unstable();
ids.dedup();
let max_parameters = max_variable_limit(connection);
for chunk in ids.chunks(max_parameters) {
let pois = load_pois_chunk(connection, chunk)?;
if pois.len() != chunk.len() {
for id in chunk {
if pois.binary_search_by_key(id, |poi| poi.id).is_err() {
return Err(SqlitePoiStoreError::MissingPoi { id: *id });
}
}
debug_assert!(false, "chunk length mismatch should reveal missing id");
}
}
Ok(())
}❌ New issue: Deep, Nested Complexity |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 1484 🏁 Script executed: Length of output: 471 🏁 Script executed: Length of output: 353 @leynos: Yes, both issues have been resolved:
Both recommendations have been implemented in the current PR state. |
This comment was marked as resolved.
This comment was marked as resolved.
1 participant
Summary
Testing
https://chatgpt.com/codex/tasks/task_e_68e0c62490bc8322bf665a428dc97e70
Summary by Sourcery
Implement a SqlitePoiStore that loads a versioned R*-tree spatial index and hydrates POIs from a read-only SQLite database for fast bounding box queries, along with test support, documentation updates, and required dependency additions.
New Features:
Enhancements:
Build:
Documentation:
Tests: