support for carray?

[c-cube]

https://sqlite.org/carray.html looks to be the best way to parametrize queries/run statements with a variadic number of arguments. For example, a select x from y where z in (<a list of values here>). It seems like the API would be reasonably compact. What do you think?

[mmottl]

Please feel free to submit a PR for any solutions, or close the issue if this is not relevant anymore.

[BraulioVM]

I've started working on this. I have some questions on the approach which I guess you may have opinions on. I don't have any experience with the C FFI, so maybe some of what I'm going to say will be a bit silly - apologies for that.

Interface

A design goal of mine (which may be overkill - let me know) was to avoid having to copy the arrays passed by the user. For this reason, I was imagining that, at least for int32s and int64s, the user would pass Bigarrays with a C layout.

We would introduce a Carray.t with an interface like

module CArray : sig
  type t

  val of_int64_bigarray : (Int64.t, Bigarray.int64_elt, Bigarray.c_layout) Bigarray.Array1.t -> t
end

(I'm sticking to int64s for the sake of the example, but I expect to add support for int32s, int64s, doubles and blobs).

Then, we would have the following additional interface:

val bind_carray : stmt -> int -> CArray.t -> Rc.t
(** [bind_carray stmt pos carray] binds the carray [carray] to the parameter at
    position [pos] of the statement [stmt] as a blob.

    @return the return code of this operation.

    @raise RangeError if [pos] is out of range.
    @raise SqliteError if the statement is invalid. *)

which would under the hood call the following sqlite3_carray_bind API passing the data for the array without copying it.

Lifetimes

However, because we are avoiding copies, we have to be careful to make sure that the CArray.t will outlive the stmt object that it's bound to. But stmt is currently represented as a custom block, so IIUC it's not currently possible to have a stmt specify lifetime dependencies on other ocaml values (the CArray.t) in any way that is legible to the GC. So we would have to represent stmt objects differently - perhaps with a record like:

type c_stmt
type stmt = {
  mutable dependent_carrays: CArray.t list;
  c_stmt: c_stmt;
}

where c_stmt is represented by the same custom block we were using previously, but we've introduced a dependent_carrays just to make the lifetime dependency between a stmt and its bound carrays visible to the GC. Functions like bind_carray or reset would make sure to either add elements to this dependent_carrays list or remove them altogether. To be clear: this new definition of stmt would be part of the private implementation of the module.

Maybe there are better ways to satisfy this lifetime dependency? Please let me know.

Packaging

The carray extension source files (carray.h and carray.c) are not typically packaged with sqlite3 packages. See this example from debian's libsqlite3-dev package. In fact, no debian package contains the files.

I don't know how this kind of problem is typically solved in the ocaml ecosystem. Is it fine to vendor the source files in this repository? Would we want to build the extension by default for all users of the package? Or should carray support be opt-in?

[mmottl]

Thanks for looking into this. Though it would be possible to ship this extension, it adds additional complexity and technical debt. E.g. upstream changes would have to be merged.

Lifetimes would have to be handled by parameter index. E.g. what if a CArray parameter gets rebound? Also, many of the current stmt functions would need to use an indirection.

If you really want to experiment with this, maybe consider extending the C-stubs to reduce changes to the .ml file. The stmt_wrap struct might be suitable for storing pointers to OCaml values, which would, of course, have to be registered with the runtime and unregistered whenever the statement gets finalized.

This is one of those things that are hard to judge before there is a proposed implementation. I believe the extension is fairly stable and serves a useful purpose. If you have a real need for it, first make it work well for yourself. That way it's much more likely others would like that solution, too, in which case I'd be happy to merge it.

[BraulioVM]

First of all,

If you have a real need for it, first make it work well for yourself. That way it's much more likely others would like that solution, too, in which case I'd be happy to merge it.

I don't actually have a need for this, which I guess may be kind of a red flag. I was interested in this issue because it seemed like a nice chance to learn more about sqlite, ocaml & its FFI.

I'm aware that supporting this extension adds to the module's maintenance cost, and that merging my PR without feedback from potential users is risky and probably not worth your maintenance cost. In that sense, I'm perfectly happy not merging the PR. My main motivation was learning, and I've got some of that value whether we merge or not.

That said, from my perspective, it'd be more rewarding if I knew this code was going to be used by someone. I'll reach out to @c-cube to see if he's interested. Enough time has passed since the creation of the issue that the answer may just be "no", and that's fine, in which case I'm happy to just close the PR.

Lifetimes would have to be handled by parameter index. E.g. what if a CArray parameter gets rebound?

Yes

consider extending the C-stubs to reduce changes to the .ml file. The stmt_wrap struct might be suitable for storing pointers to OCaml values, which would, of course, have to be registered with the runtime and unregistered whenever the statement gets finalized.

Ok, that's a much better approach than what I was considering at first. I've implemented an approach like that one in #66

Also, many of the current stmt functions would need to use an indirection.

I guess this only applies to the approach I was describing in my first comment, but the need for the indirection goes away (as long as carrays are not actually being used) if we put the carray pointers inside stmt_wrap, as I did in my PR above.