Pass reserver_ids to SQLite to pre-filter the queried chunks#138
Pass reserver_ids to SQLite to pre-filter the queried chunks#138ReinierMaas wants to merge 3 commits into
reserver_ids to SQLite to pre-filter the queried chunks#138Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces a SQLite user-defined function (UDF) to let SQLite pre-filter out already-reserved chunks during chunk selection, leveraging in-process FFI to consult the Rust Reserver state while the query executes.
Changes:
- Add
opsqueue_is_reserved(submission_id, chunk_index)filtering to allStrategychunk-selection queries and register the UDF on SQLite connections before fetching. - Expose
Reserver::is_reservedand implement the SQLite UDF callback + destructor wiring (including tests verifying reserved chunks are excluded). - Minor supporting fixes:
lookup_id_by_prefixparameter cleanup andChunkIndex: TryFrom<i64>for converting SQLite integers safely.
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| opsqueue/src/consumer/strategy.rs | Adds opsqueue_is_reserved(...) = 0 filters to strategy queries; updates query-plan tests with a no-op UDF registration. |
| opsqueue/src/consumer/dispatcher/reserver.rs | Adds Reserver::is_reserved used by the SQLite UDF callback. |
| opsqueue/src/consumer/dispatcher/mod.rs | Registers the SQLite UDF per acquired reader connection; implements the FFI callback/destructor; adds an integration-style test asserting reserved chunks are excluded. |
| opsqueue/src/common/submission.rs | Simplifies lookup_id_by_prefix to reuse $1 and avoids redundant bindings. |
| opsqueue/src/common/chunk.rs | Adds TryFrom<i64> for ChunkIndex to support parsing SQLite int64 values in the UDF. |
| opsqueue/Cargo.toml | Adds optional libsqlite3-sys dependency gated behind server-logic. |
| Cargo.lock | Records the new libsqlite3-sys dependency resolution. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| fn try_from(value: i64) -> Result<Self, Self::Error> { | ||
| if value < 0 { | ||
| return Err(crate::common::errors::TryFromIntError(())); |
There was a problem hiding this comment.
The error could include more information ? Such as the source value and target type?
There was a problem hiding this comment.
Yes it could. We haven't done that for any of the other integer errors.
| use super::strategy; | ||
| use crate::common::StrategicMetadataMap; | ||
|
|
||
| unsafe extern "C" fn sqlite_reserved_chunk_lookup( |
There was a problem hiding this comment.
Some documentation would be useful here, in particular about the parameters: as the parameter names and types are quite general.
| .unwrap(); | ||
|
|
||
| let mut conn = db_pools.reader_conn().await.unwrap(); | ||
| register_reserved_lookup_noop(conn.get_inner()).await; |
There was a problem hiding this comment.
Why is this noop being performed?
There was a problem hiding this comment.
If you don't register any function you can't call into SQLite to explain the plan even though you don't execute the query it checks that the called function is provided.
This uses the fact that SQLite query engine lives in the same process as the calling Rust code to pass the integers directly into the query via SQL-FFI.
This can be extended to the
metastateas well so we don't need to serialize to JSON in order to use the data in the query itself.