refactor: Migrate sqlite from adapter::kv to Access instead#6328
Merged
Xuanwo merged 2 commits intoapache:mainfrom Oct 22, 2025
Merged
refactor: Migrate sqlite from adapter::kv to Access instead#6328Xuanwo merged 2 commits intoapache:mainfrom
Xuanwo merged 2 commits intoapache:mainfrom
Conversation
dosubot
bot
added
size:XL
Member
|
Hi, please fix the CI issues. |
NoxTav
force-pushed
the
refactor-sqlite
branch
10 times, most recently
from
292c736 to
6986d0c
Compare
Contributor
Author
|
@Xuanwo I fixed the CI. |
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the SQLite service to use the new Access trait instead of the old adapter::kv approach.
- Introduce
SqliteAccessorimplementing full CRUD and list operations via theAccesstrait. - Add dedicated modules for writer (
SqliteWriter), lister (SqliteLister), and deleter (SqliteDeleter). - Remove the old
kv::Adaptercode and wire up the builder to produce the new accessor, plus add initial tests.
Reviewed Changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| core/src/services/sqlite/writer.rs | Adds SqliteWriter for buffered writes, close and abort logic |
| core/src/services/sqlite/lister.rs | Adds SqliteLister for recursive and non-recursive listings |
| core/src/services/sqlite/delete.rs | Adds SqliteDeleter for one-shot deletes |
| core/src/services/sqlite/mod.rs | Exposes the new modules under the services-sqlite feature flag |
| core/src/services/sqlite/core.rs | Implements SqliteCore with get, get_range, set, and delete APIs |
| core/src/services/sqlite/backend.rs | Replaces kv::Adapter with SqliteAccessor, wires up builder, adds tests |
| core/Cargo.toml | Adds tempfile dependency for test setup |
Comments suppressed due to low confidence (3)
core/src/services/sqlite/backend.rs:156
- The root configured via the builder is ignored here—
SqliteAccessor::newalways defaults to "/. You should apply the normalizedrootvalue, e.g.,.with_normalized_root(root)`, so builder settings take effect.
Ok(SqliteAccessor::new(SqliteCore {
core/src/services/sqlite/backend.rs:301
- The test suite covers accessor creation and builder settings but doesn't include tests for core operations (
stat,read,write,delete,list). Consider adding tests for these to ensure end-to-end functionality.
mod test {
core/src/services/sqlite/core.rs:27
- [nitpick] Consider adding documentation comments for
SqliteCoreto explain its purpose and how its methods (get, get_range, set, delete) are expected to be used.
pub struct SqliteCore {
Xuanwo
reviewed
Jul 11, 2025
core/src/services/sqlite/backend.rs
Outdated
| } | ||
|
|
||
| #[test] | ||
| fn test_sqlite_builder_interface() { |
core/src/services/sqlite/lister.rs
Outdated
| } else { | ||
| // Non-recursive listing under a specific path | ||
| (format!( | ||
| "SELECT `{}` FROM `{}` WHERE (`{}` LIKE $1 OR `{}` = $2) AND (`{}` NOT LIKE $3 OR (`{}` LIKE $4 AND `{}` NOT LIKE $5))", |
Member
There was a problem hiding this comment.
Should we just use something like
SELECT key
FROM objects
WHERE key GLOB 'a/*'
AND key NOT GLOB 'a/*/*';
Contributor
Author
There was a problem hiding this comment.
GLOB is good for simple cases, but it is not precise enough for more complex cases like LibSubDir. So we need LIKE with a complex query.
NoxTav
force-pushed
the
refactor-sqlite
branch
6 times, most recently
from
91809a5 to
ef873bc
Compare
Contributor
Author
|
@Xuanwo Up |
Member
|
Thanks for working on this, but I’m not fond of how |
NoxTav
force-pushed
the
refactor-sqlite
branch
from
ef873bc to
3294097
Compare
NoxTav
force-pushed
the
refactor-sqlite
branch
2 times, most recently
from
a9ec78c to
7f07a63
Compare
NoxTav
force-pushed
the
refactor-sqlite
branch
from
7f07a63 to
079e786
Compare
Contributor
Author
|
@Xuanwo I removed the |
Xuanwo
reviewed
Oct 22, 2025
core/Cargo.toml
Outdated
| sha2 = "0.10" | ||
| size = "0.5" | ||
| tokio = { version = "1.48", features = ["fs", "macros", "rt-multi-thread"] } | ||
| tempfile = "3.20.0" |
Member
There was a problem hiding this comment.
We don't want to add new deps like tempfile in opendal, can we avoid that?
core/src/services/sqlite/backend.rs
Outdated
| Ok(stream) | ||
| async fn build_client() -> OnceCell<SqlitePool> { | ||
| let file = tempfile::NamedTempFile::new().unwrap(); | ||
| let config = SqliteConnectOptions::from_str(file.path().to_str().unwrap()).unwrap(); |
Member
There was a problem hiding this comment.
sqlite should be able to run in memory?
NoxTav
force-pushed
the
refactor-sqlite
branch
from
079e786 to
7e9d5a4
Compare
Xuanwo
approved these changes
Oct 22, 2025
Member
Xuanwo
left a comment
Xuanwo
left a comment
There was a problem hiding this comment.
Thank you for working on this!
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Part #5739
Rationale for this change
See #5739
What changes are included in this PR?
Migrate sqlite from adapter::kv to Access instead
Are there any user-facing changes?
No.