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.

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

Manually preserve cconvert values until sqlite3_step #308

Merged
merged 1 commit into from
Nov 17, 2022

Conversation

quinnj
Copy link
Member

@quinnj quinnj commented Nov 17, 2022

Fixes #306.

The core issue here is that when we bind! some values to an insert statement, the _cconvert_ed value needs to remain valid (i.e. not GC-able) until we execute the insert statement in sqlite3_step. In each SQLite.Stmt, we have a params field for storing these values, but we weren't accounting for the fact that some values, (in the original issue case, InlineStrings), may be cconverted to a different value when they're actually bound, and it's the cconverted value that we need to store in params until the statement is executed.

Fixes #306.

The core issue here is that when we `bind!` some values to an insert
statement, the _cconvert_ed value needs to remain valid (i.e. not
GC-able) until we execute the insert statement in `sqlite3_step`.
In each `SQLite.Stmt`, we have a `params` field for storing these
values, but we weren't accounting for the fact that some values,
(in the original issue case, InlineStrings), may be `cconvert`ed
to a different value when they're actually bound, and it's the cconverted
value that we need to store in `params` until the statement is executed.
@codecov
Copy link

codecov bot commented Nov 17, 2022

Codecov Report

Merging #308 (f7204f7) into master (396971f) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #308   +/-   ##
=======================================
  Coverage   94.64%   94.65%           
=======================================
  Files           4        4           
  Lines         560      561    +1     
=======================================
+ Hits          530      531    +1     
  Misses         30       30           
Impacted Files Coverage Δ
src/SQLite.jl 95.95% <100.00%> (+0.01%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@bkamins
Copy link
Contributor

bkamins commented Nov 17, 2022

I have run multiple tests and all looks good (and running multiple tests I noticed #309 which I was not aware of 😄):

julia> using CSV

julia> using DataFrames

julia> using SQLite

julia> puzzles = CSV.read("puzzles.csv", DataFrame);

julia> Base.summarysize(puzzles)
534679666

julia> db = SQLite.DB("puzzles.db")
SQLite.DB("puzzles.db")

julia> @time SQLite.load!(puzzles, db, "puzzles");
  7.957707 seconds (109.19 M allocations: 2.028 GiB, 3.73% gc time, 16.18% compilation time)

julia> query = DBInterface.execute(db, "SELECT * FROM puzzles");

julia> @time puzzles_db = DataFrame(query);
 11.836739 seconds (76.27 M allocations: 2.660 GiB, 5.01% gc time, 2.91% compilation time)

julia> close(db)

@quinnj quinnj merged commit 0a8c1bf into master Nov 17, 2022
@quinnj quinnj deleted the jq-preserve-cconvert branch November 17, 2022 21:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SQLite - error regression in Julia 1.8 vs Julia 1.7
2 participants