-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
ARROW-108: [C++] Add Union implementation and IPC/JSON serialization tests #264
Conversation
…xample in IPC Change-Id: Icc5c3f9a5080d3853ed6382f714b8a1ee2595adf
…rmat because of include order sensitivity Change-Id: I5376a75ca2df817570b1e6cf929bfd64e0809333
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see that you have changed ArrayPtr
in a lot but not all places. The changes adds more characters but in the end, it's more readable because it actually tells one that it is a shared_ptr
. But then it would be better if we would get rid of it completely.
using TypeClass = UnionType; | ||
|
||
UnionArray(const TypePtr& type, int32_t length, | ||
std::vector<std::shared_ptr<Array>>& children, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
std::vector<std::shared_ptr<Array>>& children, | ||
const std::shared_ptr<Buffer>& type_ids, | ||
const std::shared_ptr<Buffer>& offsets = nullptr, int32_t null_count = 0, | ||
std::shared_ptr<Buffer> null_bitmap = nullptr); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const ref
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
Change-Id: I69146616629ba3adcee56f63133fe9bd59922c5e
Change-Id: If418d5862fbe84e0623c82fc74ecab5f89f86eef
Change-Id: I02a81e35f277fb08ad54ffafec878ff9e7b025e7
Tests are completed |
Removed ArrayPtr completely |
Change-Id: Ie49471df170207c6a9ce7be3601096e8f48d2204
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1, LGTM
Closes #206.
Still need to add test cases for JSON read/write and dense union IPC. Integration tests can happen in a subsequent PR (but the Java library does not support dense unions yet, so sparse only -- i.e. no offsets vector)