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

consider allowing usage of std::span<T> / gsl::span<T> for writing blobs #184

Open
lakinwecker opened this issue Sep 25, 2019 · 7 comments

Comments

@lakinwecker
Copy link

As the title says, it would be nice to have this an option for writing blobs

@etam
Copy link

etam commented Sep 26, 2019

and std::string_view for reading strings

@zauguin
Copy link
Collaborator

zauguin commented Oct 9, 2019

@lakinwecker Thanks for the request. We want to add a blob function, such that writing blobs will use ... << blob(some_vector) instead of ... << some_vector (#125, I will resurrect/merge it soon). This is based on the idea that especially when reading, >> vector looks like you are reading multiple rows instead of one column. Also this makes it easier to add additional container types without risking to add problems with other uses. Therefore, one the blob_t request is merged, we will add this there.

@etam I don't see how this would work. std::string_view can already be used for writing strings, but a string view doesn't own the memory it is pointing to (It is a view after all). So if we allow reading std::string_view, the memory is still owned by SQLite, leading to lots of easy to miss memory leaks. Why do you need this?

@lakinwecker
Copy link
Author

@zauguin would we want blob to operate over a span instead of a vector? That way we can use things like std::array<> or some other container that a span can wrap? ... << blob(some_span)

Done correctly, I think this would allow ... << blob(some_vector) and ... << blob(some_array) etc. Also, would this allow streaming of the blob data so we don't have to have it all in memory at once?

@etam
Copy link

etam commented Oct 9, 2019

@etam I don't see how this would work. std::string_view can already be used for writing strings, but a string view doesn't own the memory it is pointing to (It is a view after all). So if we allow reading std::string_view, the memory is still owned by SQLite, leading to lots of easy to miss memory leaks. Why do you need this?

There might be some misunderstanding about "reading" and "writing" terms. Maybe because the issue is about writing data and I wrote in my comment about reading data. I thought it's a good place, because it's all about adding support for types that fulfill some concept (yay, c++20 terminology), not just specific std::string and std::vector.

To make things clear: I want to be able to do this:

database << "some query with ?;" << string_view;

I'm also using a gsl::span<const std::uint8_t> as a buffer view in my application and I'd like to pass it to queries, like above.

@zauguin
Copy link
Collaborator

zauguin commented Oct 9, 2019

@etam Oh, that's what I considered "writing". This is already implemented in dev.

@lakinwecker Basically yes. blob basically returns a specially tagged span, called blob_t. Currently we will not use span under-the-hood to be compatible with the existing standard library, but once std::span is available this will change. Similar for the function blob: The idea is that it can be seen as a customization point which can be implemented for arbitrary blocks of data, e.g. std::span. At some point we will be able to base this on span, but for the moment it will start with overloads for common types. Similar to optional etc we can add feature detection for std::span and the overload already, but we have to be a bit cautious here because this keeps causing problems like #191... The gsl type will probably need some custom code or a magic define to avoid depending on GSL.

Streaming is a bit more complicated, mostly because the only API (AFAICT) SQLite has for streaming is the incremental BLOB I/O system. That only handles BLOB which are already stored in the table, to it can't really be used at the binding level. We could add a separate interface to expose this though. I will think about it, but I want to get blob merged first and hopefully release the dev branch soon, given that the last release is more than two years old 🙈 .
If you have a great idea how to implement this or how the interface should look like, fell free to send a PR/open a separate issue, I think it would be best to keep this focused on blobs.

@lakinwecker
Copy link
Author

Waiting until it's part of the standard is an understandable approach.

@niansa
Copy link

niansa commented Jan 7, 2023

It's part of the standard by now :-)

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

No branches or pull requests

4 participants