Skip to content

ARROW-5361: [R] Follow DictionaryType/DictionaryArray changes from ARROW-3144#4413

Closed
romainfrancois wants to merge 3 commits intoapache:masterfrom
romainfrancois:ARROW-5361/dictionary
Closed

ARROW-5361: [R] Follow DictionaryType/DictionaryArray changes from ARROW-3144#4413
romainfrancois wants to merge 3 commits intoapache:masterfrom
romainfrancois:ARROW-5361/dictionary

Conversation

@romainfrancois
Copy link
Contributor

@romainfrancois romainfrancois commented May 30, 2019

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

@romainfrancois romainfrancois requested a review from wesm May 30, 2019 06:35
@nealrichardson
Copy link
Member

nealrichardson commented May 30, 2019

This failed on Travis: https://travis-ci.org/apache/arrow/jobs/539085011#L3842-L3854

So,

  1. Looks like that may be fixed by running devtools::document() since you did update the inline docs
  2. Please remove the allow_failures from .travis.yml since R is now expected to pass: https://github.com/apache/arrow/blob/master/.travis.yml#L50-L51

Otherwise, once Travis is truly green again, LGTM.

@wesm
Copy link
Member

wesm commented May 30, 2019

@romainfrancois can you remove R from the allow_failures section of Travis CI?

https://github.com/apache/arrow/blob/master/.travis.yml#L50

@romainfrancois romainfrancois force-pushed the ARROW-5361/dictionary branch from e557486 to 1d3f6c2 Compare May 31, 2019 06:57
@romainfrancois
Copy link
Contributor Author

romainfrancois commented May 31, 2019

I'm currently getting errors on travis that don't seem related to r/ I guess the R job needs some attention.

@romainfrancois romainfrancois mentioned this pull request May 31, 2019
Closed
@romainfrancois romainfrancois force-pushed the ARROW-5361/dictionary branch from 1547438 to 1d3f6c2 Compare May 31, 2019 14:41
@nealrichardson
Copy link
Member

nealrichardson commented May 31, 2019

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.

@nealrichardson
Copy link
Member

nealrichardson commented May 31, 2019

Ok @romainfrancois #4443 is in master now so if you merge/rebase, this should pass 🤞

@romainfrancois romainfrancois force-pushed the ARROW-5361/dictionary branch from 1d3f6c2 to b0de1a8 Compare June 1, 2019 07:36
@codecov-io
Copy link

codecov-io commented Jun 1, 2019

Codecov Report

Merging #4413 into master will decrease coverage by <.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            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
Impacted Files Coverage Δ
cpp/src/parquet/properties.h 94.95% <0%> (-1.48%) ⬇️
go/arrow/ipc/writer.go 76.56% <0%> (-1.4%) ⬇️
cpp/src/arrow/util/io-util.cc 74.82% <0%> (-1.35%) ⬇️
js/src/ipc/metadata/message.ts 92.99% <0%> (-1.28%) ⬇️
go/arrow/ipc/file_reader.go 59.64% <0%> (-1.08%) ⬇️
js/src/schema.ts 88.77% <0%> (-1.03%) ⬇️
cpp/src/arrow/flight/test-util.cc 88.72% <0%> (-0.84%) ⬇️
cpp/src/arrow/python/flight.cc 87.69% <0%> (-0.77%) ⬇️
cpp/src/arrow/flight/server.cc 90.06% <0%> (-0.54%) ⬇️
cpp/src/parquet/arrow/writer.cc 96.44% <0%> (-0.5%) ⬇️
... and 70 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update aa18d25...b0de1a8. Read the comment docs.

@romainfrancois romainfrancois deleted the ARROW-5361/dictionary branch June 3, 2019 07:08
@romainfrancois romainfrancois mentioned this pull request Jun 10, 2019
Closed
@asfimport asfimport mentioned this pull request Jun 3, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@wesm wesm Awaiting requested review from wesm

Assignees

No one assigned

Labels

Component: R ready-for-review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@romainfrancois @nealrichardson @wesm @codecov-io