ARROW-4244: [Format] Clarify padding/alignment rationale/recommendation.#3388
ARROW-4244: [Format] Clarify padding/alignment rationale/recommendation.#3388emkornfield wants to merge 4 commits into
Conversation
The flow of text was a little bit strange. I think this was because of change from 64 bytes required padding/alignment to a recommendation and using 8 byte padding/alignment as the requirement.
| an aligned 8-byte offset. Additionally, each buffer should be padded to a multiple | ||
| of 8 bytes. | ||
| * The general recommendation is to align buffers to a 64-byte boundary and pad | ||
| to a multiple of 64 bytes, but this is not absolutely necessary. |
There was a problem hiding this comment.
Shall we emphasis on highly recommended?
There was a problem hiding this comment.
Possibly, I couldn't find the thread that was the impetus to the change in requirement, so I'm not sure what the impetus was.
It looks like C++ and Rust implementations currently honor 64-bytes padding and alignment. C# has 64 byte alignment and 8 byte padding. I couldn't determine what the Java implementation was using (and I didn't look at javascript).
There was a problem hiding this comment.
Added bold and a note on performance reasons, and put the recommendation in bold. Let me know if you would like further changes.
Codecov Report
@@ Coverage Diff @@
## master #3388 +/- ##
=========================================
+ Coverage 87.62% 89.3% +1.67%
=========================================
Files 464 502 +38
Lines 54471 70343 +15872
=========================================
+ Hits 47729 62817 +15088
- Misses 6633 7526 +893
+ Partials 109 0 -109
Continue to review full report at Codecov.
|
wesm
left a comment
wesm
left a comment
There was a problem hiding this comment.
+1. Thank you, this makes things clearer / more consistent
wesm
changed the title
4 participants
I think this makes it the spec more consistent (but I might have misunderstood the intent).