Skip to content

ARROW-5505: [R] Stop masking base R functions - #5172

Closed
nealrichardson wants to merge 4 commits into
apache:masterfrom
nealrichardson:r-to-arrow
Closed

ARROW-5505: [R] Stop masking base R functions#5172
nealrichardson wants to merge 4 commits into
apache:masterfrom
nealrichardson:r-to-arrow

Conversation

@nealrichardson

Copy link
Copy Markdown
Member

New attempt at this. It replaces table() and array() with to_arrow(), which already existed but was not exported.

Review much appreciated @romainfrancois

@codecov-io

Copy link
Copy Markdown

Codecov Report

Merging #5172 into master will decrease coverage by 12.43%.
The diff coverage is 100%.

Impacted file tree graph

@@ Coverage Diff @@## master #5172 +/- ##
==========================================
- Coverage 87.63% 75.2% -12.44% 
==========================================
Files 1022 58 -964 Lines 146258 3621 -142637 Branches 1437 0 -1437 ==========================================
- Hits 128180 2723 -125457 + Misses 17716 898 -16818 + Partials 362 0 -362
Impacted FilesCoverage Δ
r/R/Table.R85% <ø> (-3%)⬇️
r/R/write_arrow.R100% <ø> (+3.44%)⬆️
r/R/array.R62.5% <ø> (-1.14%)⬇️
r/R/to-arrow.R100% <100%> (ø)
r/src/table.cpp75.75% <0%> (-5.06%)⬇️
python/pyarrow/ipc.pxi
cpp/src/parquet/column_page.h
cpp/src/plasma/test/external_store_tests.cc
cpp/src/arrow/array/builder_decimal.cc
cpp/src/plasma/client.cc
... and 961 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 c050244...209f182. Read the comment docs.

@romainfrancois

Copy link
Copy Markdown
Contributor

I get the need to not masking base functions and this is something we have come to regret in other projects. I was trying here to mimic the 🐍 interface (pa.array() ...)

to_arrow() feels vague, maybe we could do some sort of namespacing, e.g. arrow_array() or something, but then we would kind of have to do it for all functions :/

@wesm

wesm commented Sep 7, 2019

Copy link
Copy Markdown
Member

This has a rebase conflict, what's the status of this?

@nealrichardson

Copy link
Copy Markdown
MemberAuthor

Closing; this is superseded by #5279.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@nealrichardson@codecov-io@romainfrancois@wesm