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

relax bind! Vector{UInt8} -> AbstractVector{UInt8} #340

Merged
merged 5 commits into from
May 22, 2024

Conversation

maxfreu
Copy link
Contributor

@maxfreu maxfreu commented Apr 3, 2024

This allows zero-copy writes e.g. for ReinterpretArrays.

Do you want this to be tested explicitly?

This allows zero-copy writes e.g. for ReinterpretArrays.
@quinnj
Copy link
Member

quinnj commented Apr 14, 2024

Sorry for the slow review on this; I'd like to merge, but can you look at the failing test and see if we should update the test or if we need something else to maintain desired behavior?

@ScottPJones
Copy link

ScottPJones commented Apr 14, 2024

I'd be afraid that this would lose a requirement that the Vector be contiguous in memory (which is I believe true for the underlying SQLite C code).

You might need to add a new method, that specifically takes Base.ReinterpretArray, but only with <: DenseVector.

@maxfreu
Copy link
Contributor Author

maxfreu commented Apr 15, 2024

Sooo: I'm trying to understand what happens in the test. Here

b"bytearray",
we write a bytearray of type Base.CodeUnits{UInt8, String} to a newly created table in a BLOB column. Before this change, the bytearray was written as a serialized julia object, including type information. So after reading it back in, its type is completely restored, but at the cost of storing it in a julia-only readable format (not as plain bytearray). After this patch, the bytes corresponding to the string "bytearray" are written. After reading them back in, it's just a plain vector. In consequence, the test would have to be changed.

I'd be afraid that this would lose a requirement that the Vector be contiguous in memory (which is I believe true for the underlying SQLite C code).

Ok if that is true, we have to take it seriously. So far it has worked for me. Can you point to a resource for more information?

@maxfreu
Copy link
Contributor Author

maxfreu commented Apr 15, 2024

Ok, I just implemented a non-contiguous AbstractVector and confirmed that it does not work. Implementing Base.iterate and getindex is not enough, you have to specify the conversion to a pointer - and that's just impossible for non-contiguous data.

I'll come up with sth along the lines you suggested.

@maxfreu
Copy link
Contributor Author

maxfreu commented Apr 16, 2024

Can you trigger tests again?

@quinnj
Copy link
Member

quinnj commented Apr 20, 2024

Looks like this is getting closer. There's a failure on the nightly CI though w/ trying to get a pointer to ReinterpretArray. I'm not sure if there's a valid way to do that after the Memory changes in Base julia? It might be worth asking around (public julia slack/zulip/discord) or snooping around the ReinterpretArray code in base to see what's possible.

@maxfreu
Copy link
Contributor Author

maxfreu commented Apr 21, 2024

I now pass a pointer to the ccall directly, so it doesn't rely on the broken conversion. That seems to work with old and new versions.

@maxfreu
Copy link
Contributor Author

maxfreu commented Apr 21, 2024

...aaand replaced pointer by Ref, because pointer is not safe from GC.

@maxfreu
Copy link
Contributor Author

maxfreu commented May 21, 2024

@quinnj ping :)

Copy link
Member

@quinnj quinnj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @maxfreu!

@quinnj quinnj merged commit e3c6814 into JuliaDatabases:master May 22, 2024
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.

3 participants