Closed
Conversation
Merged
Contributor
|
@palday - can you please rebase this PR against |
Contributor
Author
|
@bkamins They don't pass on M1 yet -- I was mostly just pushing what I had so far in order to reduce duplication of effort. In other words, it's definitely not ready for merge. |
Member
|
Thanks for starting this PR @palday; it looks like the file case is failing specifically. I'll try to look into it a little later tonight. |
Member
|
Sorry for the slow response here; I've been digging into this, but it's turning into a rabbit hole where I suspect there's actually a Julia core issue happening? I'll keep digging today. |
quinnj
added a commit
that referenced
this pull request
Nov 3, 2022
Fixes #345. Alternative to #350. Whew, a bit of a rabbit hole here. It turns out this issue doesn't have anything to do w/ the _arrow_ alignment when writing, but actually goes back to a core Julia issue, specifically the inconsistency reported in JuliaLang/julia#42326. Essentially, the issue is that different platforms (intel vs. arm64) are requiring different alignments for types like `UInt128` (8 for intel, 16 for arm64). And it turns out this isn't even a Julia issue, but all the way back to LLVM. So why does this crop up in Arrow.jl? Because when you try to serialize/deserialize a `Arrow.Primitive{Int128}` array, we're going to write it out in the proper arrow format, but when you read it back in, we've been using the zero-copy technique of: ```julia unsafe_wrap(Array, Ptr{Int128}(arrow_ptr), len) ``` in order to give the user a Julia `Vector{Int128}` to the underlying arrow data. _BUT_, in Julia when you allocate any `Vector` with an eltype size of less than 4, then only 8 bytes of alignment are specified. So the fact that most users will pass arrow data as a file (which is mmapped as `Vector{UInt8}`), a byte vector directly (`Vector{UInt8}`), or an IO (which we read as `Vector{UInt8}`), means these vectors are only 8-byte aligned. This then throws the fatal error reported in the original #350 issue about the pointer not being 16-byte aligned. So one option to consider is allowing users to pass in a 16-byte aligned arrow "source" and then that would just work, right? Well, except Julia doesn't expose any way of "upcasting" an array's alignment, so it's purely based off the array eltype. Which in turn is a struggle because the current Arrow.jl architecture assumes the source will be exactly a `Vector{UInt8}` everywhere. So essentially it requires some heruclean effort to try and pass your own aligned array; I think the only option I can think of is if you did something like: * Mmap an arrow file into Vector{UInt8} * Allocate your own Vector{UInt32} and copy arrow data into it * Use unsafe_wrap to make a Vector{UInt8} of the Vector{UInt32} data * But you _MUST_ keep a reference to the original Vector{UInt32} array otherwise, your new Vector{UInt8} gets corrupted * Pass the new Vector{UInt8} into Arrow.Table to read Anyway, not easy to say the least. The proposed solution here is as follows: * We check `sizeof(T)` to see if it's > 16 bytes, which we're using as a proxy for the alignment, regardless of platform (core devs are leaning towards the 8-byte alignment on intel actually being a bug for 16-byte primitives and I agree) * We use sizeof since the `jl_datatype_t` alignment field isn't part of the public Julia API and thus subject to change (and in fact I think it did just change location in Julia#master) * It's a good enough proxy for our purposes anyway * If the original arrow pointer _isn't_ 16-byte aligned, then we'll allocate a new `Vector{T}`, which _will_ be aligned, then copy the arrow data directly into it via pointers. Simple, easy, just one extra allocation/copy. If Julia _does_ get the ability in the future to specify a custom larger-than-eltype-required alignemnt for arrays, then we could potentially do that ourselves when reading, but it's a little tricky because we really only need to do that if there are 16-byte primitives we'll be deserializing and we don't know that until we read a schema message. So *shrug*.
Member
|
Whew, rabbit hole plunged: #357 |
quinnj
added a commit
that referenced
this pull request
Nov 3, 2022
Fixes #345. Alternative to #350. Whew, a bit of a rabbit hole here. It turns out this issue doesn't have anything to do w/ the _arrow_ alignment when writing, but actually goes back to a core Julia issue, specifically the inconsistency reported in JuliaLang/julia#42326. Essentially, the issue is that different platforms (intel vs. arm64) are requiring different alignments for types like `UInt128` (8 for intel, 16 for arm64). And it turns out this isn't even a Julia issue, but all the way back to LLVM. So why does this crop up in Arrow.jl? Because when you try to serialize/deserialize a `Arrow.Primitive{Int128}` array, we're going to write it out in the proper arrow format, but when you read it back in, we've been using the zero-copy technique of: ```julia unsafe_wrap(Array, Ptr{Int128}(arrow_ptr), len) ``` in order to give the user a Julia `Vector{Int128}` to the underlying arrow data. _BUT_, in Julia when you allocate any `Vector` with an eltype size of less than 4, then only 8 bytes of alignment are specified. So the fact that most users will pass arrow data as a file (which is mmapped as `Vector{UInt8}`), a byte vector directly (`Vector{UInt8}`), or an IO (which we read as `Vector{UInt8}`), means these vectors are only 8-byte aligned. This then throws the fatal error reported in the original #350 issue about the pointer not being 16-byte aligned. So one option to consider is allowing users to pass in a 16-byte aligned arrow "source" and then that would just work, right? Well, except Julia doesn't expose any way of "upcasting" an array's alignment, so it's purely based off the array eltype. Which in turn is a struggle because the current Arrow.jl architecture assumes the source will be exactly a `Vector{UInt8}` everywhere. So essentially it requires some heruclean effort to try and pass your own aligned array; I think the only option I can think of is if you did something like: * Mmap an arrow file into Vector{UInt8} * Allocate your own Vector{UInt32} and copy arrow data into it * Use unsafe_wrap to make a Vector{UInt8} of the Vector{UInt32} data * But you _MUST_ keep a reference to the original Vector{UInt32} array otherwise, your new Vector{UInt8} gets corrupted * Pass the new Vector{UInt8} into Arrow.Table to read Anyway, not easy to say the least. The proposed solution here is as follows: * We check `sizeof(T)` to see if it's > 16 bytes, which we're using as a proxy for the alignment, regardless of platform (core devs are leaning towards the 8-byte alignment on intel actually being a bug for 16-byte primitives and I agree) * We use sizeof since the `jl_datatype_t` alignment field isn't part of the public Julia API and thus subject to change (and in fact I think it did just change location in Julia#master) * It's a good enough proxy for our purposes anyway * If the original arrow pointer _isn't_ 16-byte aligned, then we'll allocate a new `Vector{T}`, which _will_ be aligned, then copy the arrow data directly into it via pointers. Simple, easy, just one extra allocation/copy. If Julia _does_ get the ability in the future to specify a custom larger-than-eltype-required alignemnt for arrays, then we could potentially do that ourselves when reading, but it's a little tricky because we really only need to do that if there are 16-byte primitives we'll be deserializing and we don't know that until we read a schema message. So *shrug*.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
closes #345
So far, this only changes the default alignment. But that isn't sufficient because apparently that alignment isn't respected everywhere.