Add C API to unset all query buffers.

[teo-tsirpanis]

This PR adds new C and C++ APIs that unset all previously set query buffers. It can be useful to provide additional memory safety by guaranteeing that the Core will not hold references to user buffers after a certain point, which will be used by TileDB-rs to safely decouple the buffers' lifetime from the query object (CORE-502).

---

TYPE: C_API
DESC: Add tiledb_query_unset_buffers function, that unsets all previously set buffers.

---

TYPE: CPP_API
DESC: Add Query::unset_buffers function, that unsets all previously set buffers.

[teo-tsirpanis]

I had the idea of not accepting a context here, because this API can only fail if called with a null pointer. Let me know what you think.

[rroelke]

Makes sense to me

[rroelke]

This implementation looks fine to me - but I wonder if it's the right way to solve the problem, which as I understand it is that we can't detach buffers from the Rust query object in order to fill them with the next round of global order write data.

I've suggested before that we tried replacing the tiledb-rs query buffer API with Arrow to help with this problem. That's a very high-altitude fruit. Maybe there's a lower-hanging fruit which would be a better way to address this, just from the Rust side.

fn set_buffer<'data>(&mut Self, field: &str, &'data mut [u8]) -> BufferGuard<'data>;

This pattern didn't occur to us 2 years ago; but now maybe it's something we can try. The BufferGuard mutably borrows the data buffer and keeps it attached to the query during its lifetime (so maybe we would need to unattach a specific buffer). Perhaps it could provide mutable access to the buffer as well so that the user can modify data without needing to detach/reattach to the query.

What do you think?

If there's other potential uses of this C API then I'll approve anyway.

[teo-tsirpanis]

I am not a fan of replicating the set_***_buffer API shape to Rust because I do not believe that it is the most fitting UX for reading and writing TileDB arrays. Rust makes it more evident because we could not make the first iteration of the TileDB-rs query API look like this, and there are other problems with manually setting buffers, like having to mutably borrow buffers even for write queries.

For me, the ideal UX for writing arrays would look like this (see also TileDB-Inc/TileDB-Py#2237 (comment)):

// One-shot
let array = open_array();
array.write({"attr1": data, "attr2": data2}, other_options);

// Incremental
let writer = array.start_global_order_write(options);
while (…)
{
    writer.write({"attr1": data, "attr2": data2});
}
writer.finish();

TileDB-rs is halfway through realizing this vision, and thus manually setting buffers would be IMO a step backwards.

If there's other potential uses of this C API

APIs for garbage-collected languages like C# and Go can unpin the query buffers after unsetting them from a query (e.g. after each global order write step or when resizing buffers in incomplete reads), allowing to increase memory efficiency.

[rroelke]

I am not a fan of replicating the set_***_buffer API shape to Rust because I do not believe that it is the most fitting UX for reading and writing TileDB arrays. Rust makes it more evident because we could not make the first iteration of the TileDB-rs query API look like this, and there are other problems with manually setting buffers, like having to mutably borrow buffers even for write queries.

For me, the ideal UX for writing arrays would look like this (see also TileDB-Inc/TileDB-Py#2237 (comment)):

// One-shot
let array = open_array();
array.write({"attr1": data, "attr2": data2}, other_options);

// Incremental
let writer = array.start_global_order_write(options);
while (…)
{
    writer.write({"attr1": data, "attr2": data2});
}
writer.finish();

I'll nitpick you a bit here to point out that there's an implicit map construction here which we probably ought to avoid. Or at least have the option to avoid. One idiomatic way would be to do

fn write<I>(&mut self, fields: I) -> Result where I: IntoIterator<Item = (&str, Buffer)>

The needs for write and read queries are quite different and I definitely agree with you that write queries should not need to do a mutable borrow. You didn't specifically say so but it seems to me like you would be on board to having entirely different code pathways for reads and writes.

TileDB-rs is halfway through realizing this vision, and thus manually setting buffers would be IMO a step backwards.

If there's other potential uses of this C API

APIs for garbage-collected languages like C# and Go can unpin the query buffers after unsetting them from a query (e.g. after each global order write step or when resizing buffers in incomplete reads), allowing to increase memory efficiency.

Does the query object in these languages also pin the buffers? Surely it must. Wouldn't garbage collection clear the query, and then clear the buffers after that?

I approved anyway - I'm just curious.

[teo-tsirpanis]

You didn't specifically say so but it seems to me like you would be on board to having entirely different code pathways for reads and writes.

Definitely; Query has been a god object with so many responsibilities that should be split; at least at high-level APIs first.

Does the query object in these languages also pin the buffers? Surely it must. Wouldn't garbage collection clear the query, and then clear the buffers after that?

Yes, query buffers are pinned before passing them to the C API. Because garbage collection finalizers run in a non-deterministic order, there is a single "query handle" object that manages both the tiledb_query_t* and the buffer pins. When this object gets finalized (which happens if the user has not manually called Dispose()/Free()), we call tiledb_query_free and unpin the buffers afterwards.

https://github.com/TileDB-Inc/TileDB-CSharp/blob/60f47ad9be7071b6f91b19945aeebb50f2a5e252/sources/TileDB.CSharp/Marshalling/SafeHandles/QueryHandle.cs#L54-L77

https://github.com/TileDB-Inc/TileDB-Go/blob/656934b708aa4ff8a98c3b925b80c082ab583eb8/query.go#L21-L30

---

REST-CI has succeeded on rerun (unfortunately it's not visible from this page), so merging…