-
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-5361: [R] Follow DictionaryType/DictionaryArray changes from ARROW-3144 #4413
ARROW-5361: [R] Follow DictionaryType/DictionaryArray changes from ARROW-3144 #4413
Conversation
This failed on Travis: https://travis-ci.org/apache/arrow/jobs/539085011#L3842-L3854 So,
Otherwise, once Travis is truly green again, LGTM. |
@romainfrancois can you remove R from the allow_failures section of Travis CI? https://github.com/apache/arrow/blob/master/.travis.yml#L50 |
e557486
to
1d3f6c2
Compare
I'm currently getting errors on travis that don't seem related to |
1547438
to
1d3f6c2
Compare
It's not you, it's https://issues.apache.org/jira/browse/ARROW-5470. I'm poking at a few things now to see if I can resolve it. |
Ok @romainfrancois #4443 is in master now so if you merge/rebase, this should pass 🤞 |
1d3f6c2
to
b0de1a8
Compare
Codecov Report
@@ Coverage Diff @@
## master #4413 +/- ##
==========================================
- Coverage 88.41% 88.41% -0.01%
==========================================
Files 793 792 -1
Lines 101335 101217 -118
Branches 1253 1251 -2
==========================================
- Hits 89598 89486 -112
+ Misses 11490 11483 -7
- Partials 247 248 +1
Continue to review full report at Codecov.
|
At the moment however, all the
DictionaryMemo
use is internal, it should probably be promoted to arguments (with defaults) to the R functions.I'll do this here or on another PR if this one is merged first so that
r/
builds again on travis.This now needs the C++ lib up to date, e.g. on my setup I get it through
brew install apache-arrow --HEAD
, and there is no conditional compiling so that it still works with previous versions. Let me know if that's ok.follow up from #4316