Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Tentative refactor of SQLite.jl #295

Closed
wants to merge 4 commits into from
Closed

Tentative refactor of SQLite.jl #295

wants to merge 4 commits into from

Conversation

metab0t
Copy link
Collaborator

@metab0t metab0t commented Jun 5, 2022

  1. Use Clang.jl to generate API bindings (a new C module).
  2. Separate functions to different files for clarity. (I feel that monotholic code structure is not friendly for maintainers)
  3. Use WeakKeyDict to record prepared statements of a DB, so that statements can be GCed independently from DB. (Currently SQLite.Stmt does not contain the pointer to sqlite3_stmt which is cubersome)
  4. Only ONE public API change: Change SQLite.execute to SQLite.direct_execute to avoid name confusion with DBInterface.execute.

The tests are almost unchanged and all passed on my machine.

Should fix #278 and provides a better basis for #284.

I can separate it into smaller PRs if it is needed.

@metab0t
Tentative refactor of SQLite.jl
d199ad6 
1. Use Clang.jl to generate API bindings (a new C module).
2. Separate functions to different files for clarity.
3. Use WeakKeyDict to record prepared statements of a DB, so that statements can be GCed without influence.
4. Only public API change: Change `SQLite.execute` to `SQLite.direct_execute` to avoid name confusion with `DBInterface.execute`.
@metab0t
Small fix
f22cbc1
@metab0t
Simplify sqliteerror
2d08268
@metab0t
Copy link
Collaborator Author

metab0t commented Jun 5, 2022

For SQLite.execute we can add a fallback to SQLite.direct_execute so that exposed API is nearly not affected.

@codecov
Copy link

codecov bot commented Jun 10, 2022 •
edited
Loading

Codecov Report

Merging #295 (bc222c8) into master (c1ede93) will decrease coverage by 31.20%.
The diff coverage is 51.52%.

@@             Coverage Diff             @@
##           master     #295       +/-   ##
===========================================
- Coverage   85.99%   54.78%   -31.21%     
===========================================
  Files           5       12        +7     
  Lines         707     1086      +379     
===========================================
- Hits          608      595       -13     
- Misses         99      491      +392
Impacted Files Coverage Δ
src/SQLite.jl 100.00% <ø> (+3.86%) ⬆️
src/capi.jl 15.75% <15.75%> (ø)
src/UDF.jl 85.93% <85.18%> (-0.11%) ⬇️
src/bind.jl 85.50% <85.50%> (ø)
src/base.jl 94.11% <94.11%> (ø)
src/db_and_stmt.jl 98.00% <98.00%> (ø)
src/table.jl 98.91% <98.91%> (ø)
src/db_functions.jl 100.00% <100.00%> (ø)
src/index.jl 100.00% <100.00%> (ø)
src/query.jl 100.00% <100.00%> (ø)
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c1ede93...bc222c8. Read the comment docs.

quinnj
quinnj reviewed Jun 10, 2022
View reviewed changes
test/runtests.jl
@@ -42,7 +42,7 @@ Copy test sqlite file to temp directory, path `test.sqlite`,
overwrite file if necessary and set permissions.
"""
function setup_clean_test_db(f::Function, args...)
dbfile = joinpath(dirname(pathof(SQLite)), "../test/Chinook_Sqlite.sqlite")
dbfile = joinpath(@__DIR__, "Chinook_Sqlite.sqlite")
Copy link
Member

Choose a reason for hiding this comment

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

can we revert this change? I like the pathof style since it makes it much easier for running this locally, step-wise, in the REPL.

quinnj
quinnj reviewed Jun 10, 2022
View reviewed changes
src/SQLite.jl
using Tables

include("capi.jl")
import .C as C
Copy link
Member

Choose a reason for hiding this comment

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

I think this can just be import .C since the as isn't renaming it at all. This is also causing test failures for Julia 1.3 (as didn't exist then). I'm also fine bumping the Julia version up to 1.6 though.

@quinnj
Copy link
Member

quinnj commented Jun 10, 2022

This looks great! Thanks for doing this @metab0t! I really like the auto-generated C bindings. In the future, just as an FYI, it can make reviewing much easier to avoid reorganizing files in addition to lots of changes. I don't really mind the file reorganizing, but from a git diff perspective, it can be much easier to make all the proposed changes leaving files as-is, and then do the file reorganization in a separate PR.

@metab0t
Copy link
Collaborator Author

metab0t commented Jun 10, 2022 •
edited
Loading

Thanks for reviewing!

My current plan is to separate it into 3 PRs:

  1. C bindings and other internal changes in place without influencing API. (Requires julia 1.6 LTS)
  2. File reorganization
  3. Rename or deprecate SQLite.execute

I will close this huge PR after I finish the separation.

@metab0t metab0t mentioned this pull request Jun 10, 2022
Merged
@quinnj
Copy link
Member

quinnj commented Oct 6, 2022

@metab0t , good to close this now?

@metab0t
Copy link
Collaborator Author

metab0t commented Oct 7, 2022

Yes, it can be closed.

@metab0t metab0t closed this Oct 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@quinnj quinnj quinnj left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

DBInterface.execute and SQLite.execute causes confusion
2 participants
@metab0t @quinnj