Skip to content

ARROW-459: [C++] Dictionary IPC support in file and stream formats #347

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.

Sign up for GitHub

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

Jump to bottom
Closed
wesm wants to merge 12 commits into from
Closed

ARROW-459: [C++] Dictionary IPC support in file and stream formats #347

wesm wants to merge 12 commits into from

Conversation

@wesm
Copy link
Member

@wesm wesm commented Feb 22, 2017 •
edited
Loading

Also fixes ARROW-565

@wesm wesm changed the title WIP ARROW-459: [C++] Dictionary IPC support in file and stream formats ARROW-459: [C++] Dictionary IPC support in file and stream formats Feb 23, 2017
@wesm
Copy link
Member Author

wesm commented Feb 23, 2017

OK, I've done as much as I think I should in this patch. We'll be forced to resolve any lingering issues when we get to making the integration tests run between C++/Java

xhochy
xhochy reviewed Feb 23, 2017
View reviewed changes
cpp/src/arrow/ipc/metadata.h Outdated
Copy link
Member

@xhochy xhochy Feb 23, 2017

Choose a reason for hiding this comment

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

missing a reference here

Copy link
Member Author

@wesm wesm Feb 23, 2017

Choose a reason for hiding this comment

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

done

wesm
wesm commented Feb 23, 2017
View reviewed changes
cpp/src/arrow/ipc/adapter.cc Outdated
Copy link
Member Author

@wesm wesm Feb 23, 2017

Choose a reason for hiding this comment

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

outdated comment

@wesm
Copy link
Member Author

wesm commented Feb 23, 2017

I don't know what is going on with our Travis CI build queue but I opened: https://issues.apache.org/jira/browse/INFRA-13570

@wesm
Copy link
Member Author

wesm commented Feb 23, 2017

seems to be an ASF build queue problem. the build for my latest revision is running here: https://travis-ci.org/wesm/arrow/builds/204620814

@wesm
Copy link
Member Author

wesm commented Feb 23, 2017

@xhochy for large code reviews in the future, reminder that we can always do reviews on Cloudera's Gerrit instance, where we have a project set up: https://gerrit.cloudera.org/#/admin/projects/. I've been lobbying for an ASF-administered Gerrit (so I can bug them instead of Cloudera folks when I need some admin stuff done) but that may be useful in the meantime

@wesm
Copy link
Member Author

wesm commented Feb 23, 2017

Fixing the build failures

wesm added 10 commits February 23, 2017 10:56
@wesm
Add support for deconstructing and reconstructing DictionaryArray wit…
d537004 
…h known schema

Change-Id: I888061cb9b93820eb919363d1d5c49189c2cdc0e
@wesm
Add union and dictionary to file/stream tests
6858e12 
Change-Id: I03025e3e9acefa8ac709f4425458c65155e08831
@wesm
WIP progress toward stream/file dictionary roundtrip
ae389fa 
Change-Id: Ieb848600fcb5fa506ad99d41bbad2bd9edb0f7c1
@wesm
More progress toward file/stream roundtrips with dictionaries
1cee0ff 
Change-Id: Ia673ac3de0545880751c90871a643b4d82dbb4a2
@wesm
Refactoring. Remove FileFooter class in favor of private impl in File…
cb04a41 
…Reader

Change-Id: Icb6e3fea8c09a3ad8ebf93fa0fbe4983626a9d3a
@wesm
More dictionary support in FileReader. Simple test passes
ef9dea8 
Change-Id: If15413078a1272268d17edda4b888c4de3383879
@wesm
cpplint
acfa994 
Change-Id: Ib6287480e28f6d17bcc7ea26196791bdb5a07b5b
@wesm
Add some less trivial dictionary-encoded arrays to test case
e5cec27 
Change-Id: I958f90af60d1b73283782bea0f81ec557d498d0a
@wesm
Fix Python build
7ac756e 
Change-Id: Ie8b2bf4ef5c9f7e4e5d1bc60dc231ffe2de0b8d7
@wesm
Review comments
dee044e 
Change-Id: I6104a675664c3b7fe1dfb35a68e38ec8d0035133
@xhochy
Copy link
Member

xhochy commented Feb 23, 2017

I only went briefly over this. Will have a more close look tomorrow, then I can decide if we need gerrit or not.

@wesm
Fix bug causing valgrind failure
8e0e6fb 
Change-Id: Ie87700140f26e2753ffcc865953033a67a815f0e
@wesm
Copy link
Member Author

wesm commented Feb 23, 2017

Rebased (to pick up integration test fix) and fixed valgrind error

@wesm wesm mentioned this pull request Feb 23, 2017
Closed
@wesm
Fix clang warning with forward declaration
6a987b7 
Change-Id: I6270e42a6a8f1b1ec4f08fc14e654e0677cd9f34
@wesm
Copy link
Member Author

wesm commented Feb 24, 2017

passing build in my fork: https://travis-ci.org/wesm/arrow/builds/204779749. not sure why Travis is having such a hard time

xhochy
xhochy approved these changes Feb 24, 2017
View reviewed changes
Copy link
Member

@xhochy xhochy left a comment

Choose a reason for hiding this comment

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

+1, LGTM

@asfgit asfgit closed this in d28f1c1 Feb 24, 2017
@wesm wesm deleted the ARROW-459 branch February 24, 2017 14:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@xhochy xhochy xhochy approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@wesm @xhochy