Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3933 +/- ##
=======================================
Coverage 88.77% 88.77%
=======================================
Files 25 25
Lines 2415 2415
Branches 611 606 -5
=======================================
Hits 2144 2144
Misses 269 269
Partials 2 2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
magnetised
force-pushed
the
magnetised/esqlite-sqlite-driver
branch
3 times, most recently
from
3eaee4e to
7883961
Compare
This comment has been minimized.
This comment has been minimized.
alco
left a comment
alco
left a comment
There was a problem hiding this comment.
A couple of edge cases to consider:
URI encoding may not handle all paths (sqlite3.ex:193)
"file:#{URI.encode(path)}?mode=#{mode}"URI.encode/1 may not correctly escape all SQLite URI special characters. Paths containing #, ?, or % could cause issues since these have special meaning in URIs. Consider using URI.encode(path, &URI.char_unreserved?/1) or similar to ensure all problematic characters are escaped.
convert_bind/1 may be incomplete (sqlite3.ex:202-206)
defp convert_bind(nil), do: :undefined
defp convert_bind(:null), do: :undefined
defp convert_bind(value), do: valueThis passes through most values unchanged. Does esqlite handle all the same types that exqlite did (booleans, atoms, DateTime, etc.)? May need additional conversions if exqlite was doing implicit type coercion that esqlite doesn't.
| | close(conn) | close(conn) | | ||
| | execute(conn, sql) | exec(conn, sql) | | ||
| | prepare(conn, sql) | prepare(conn, sql) | | ||
| | release(conn, stmt) | no-op – GC'd by esqlite | |
There was a problem hiding this comment.
At what point does it get GC'd? Better document this?
| | prepare(conn, sql) | prepare(conn, sql) | | ||
| | release(conn, stmt) | no-op – GC'd by esqlite | | ||
| | bind(stmt, binds) | bind(stmt, binds) | | ||
| | step(conn, stmt) | step(stmt) – conn arg dropped | |
There was a problem hiding this comment.
Where is the conn state held for this with esqlite3? Process dict or the statement itself?
There was a problem hiding this comment.
For the step/2 function? Everything's in the nif resource.
There was a problem hiding this comment.
Yes, for step. I'm unsure what the NIF resource is tied to in BEAM: is its lifetime tied to the process where the connection is open?
There was a problem hiding this comment.
I've added a (dense) para that explains how prepared statements are tied to the process memory - I asked the same questions re the lack of release for the stmts and was satisfied by the answer.
There was a problem hiding this comment.
Thanks! That helps.
I'm not too familiar with NIF resource lifetime management and how that plays with Erlang's GC. Like, if a process dies and its whole heap can be released, does the GC still scan it looking for NIF resources that need to be destroyed? It's not related to your changes, just mind-wandering.
This comment has been minimized.
This comment has been minimized.
magnetised
force-pushed
the
magnetised/esqlite-sqlite-driver
branch
from
a60c7b6 to
7a78463
Compare
|
This PR has been released! 🚀 The following packages include changes from this PR:
Thanks for contributing to Electric! |
2 participants
No description provided.