ARROW-4560: [R] array() needs to take single input, not ... #3635

romainfrancois · 2019-02-13T09:50:30Z

This will simplify the handling of the type argument.

jira: https://issues.apache.org/jira/browse/ARROW-4560?filter=12344983

romainfrancois · 2019-02-13T09:59:21Z

Also planning to work on https://issues.apache.org/jira/browse/ARROW-3810 in this PR

romainfrancois · 2019-02-14T13:07:50Z

To handle the type= argument in array() we need to be able to infer arrow types from R objects, so I've added the type() function:

library(arrow, warn.conflicts = FALSE)

type(1:10)
#> arrow::Int32 
#> int32
type(1)
#> arrow::Float64 
#> double
type("")
#> arrow::Utf8 
#> string
type(iris$Species)
#> arrow::DictionaryType 
#> dictionary<values=string, indices=int8, ordered=0>

^{Created on 2019-02-14 by the reprex package (v0.2.1.9000)}

romainfrancois · 2019-02-20T09:26:26Z

I'm now fairly confident about this PR. array() used to take ... and rely on vctrs to first combine to a common type using vctrs type system, though the .ptype. I've changed that so that array() only take one vector x and the type= argument is an arrow logical type, e.g. int32().

library(arrow, warn.conflicts = FALSE)

a <- array(1:10, type = int64())
a$type
#> arrow::Int64 
#> int64
a$as_vector()
#> integer64
#>  [1] 1  2  3  4  5  6  7  8  9  10

a <- array(1:10, type = float64())
a$type
#> arrow::Float64 
#> double
a$as_vector()
#>  [1]  1  2  3  4  5  6  7  8  9 10

The type of array that is made is governed by the type= argument. If missing, the type is inferred from the data, e.g.

library(arrow, warn.conflicts = FALSE)

a <- array(rnorm(10))
a$type
#> arrow::Float64 
#> double
a$as_vector()
#>  [1]  0.48428301  0.74399694  0.48991163  0.35701469  0.60517996
#>  [6]  0.53463545  0.12579375  0.02414986 -0.49127583  0.17194012

The chunked_array() factory handles ... and a type argument too:

library(arrow, warn.conflicts = FALSE)

a <- chunked_array(rnorm(10), 1:10)
a$type
#> arrow::Float64 
#> double
a$as_vector()
#>  [1]  1.64642028 -1.47423016  0.84996187  1.21151724 -1.52303727
#>  [6] -0.04387242 -0.47798708 -0.18693768 -0.98903429  0.30376938
#> [11]  1.00000000  2.00000000  3.00000000  4.00000000  5.00000000
#> [16]  6.00000000  7.00000000  8.00000000  9.00000000 10.00000000

a <- chunked_array(1:5, 1:10, type = int64())
a$type
#> arrow::Int64 
#> int64
a$as_vector()
#> integer64
#>  [1] 1  2  3  4  5  1  2  3  4  5  6  7  8  9  10

… factors/dictionaries

…since array() no longer deals with ...

…all arrays end up with the same type

wesm

+1, this looks nice. I'm going to rebase to fix any linting issue since I just added the cpplint checks

wesm · 2019-02-27T23:14:44Z

r/tests/testthat/test-type.R

+
+test_that("type() infers from R type", {
+  expect_equal(type(1:10), int32())
+  expect_equal(type(1), float64())


The optics of this are a bit odd =)

yeah maybe that can be infer_type() or something

r/tests/testthat/test-type.R

wesm · 2019-02-27T23:15:23Z

r/tests/testthat/test-type.R

+  expect_equal(type(""), utf8())
+  expect_equal(
+    type(iris$Species),
+    dictionary(int8(), array(levels(iris$Species)), FALSE)


maybe test for ordered factors at some point also?

wesm · 2019-02-27T23:18:43Z

Oops I missed array_from_vector.cpp in my code review because it was hidden. silly GitHub. I'll take a skim through anyway

wesm

Some minor comments. I'll do a better job reviewing next time...

wesm · 2019-02-27T23:20:29Z

r/src/array_from_vector.cpp

+    null_bitmap_writer.Finish();
+  }
+
+  // ----- data buffer


It is efficient to visit the string elements twice? I think you should have some benchmarks about this

This is so we only AllocateBuffer the buffer with the final size we need. The LENGTH() here is quick, R strings know their size: https://purrple.cat/blog/2018/03/05/strings-know-their-own-length/

Is the alternative to grow a buffer as we go ? Maybe there is a StringArrayBuilder I can use

wesm · 2019-02-27T23:21:25Z

r/src/array_from_vector.cpp

+    // catch up
+    for (R_xlen_t j = 0; j < i; j++, null_bitmap_writer.Next()) {
+      null_bitmap_writer.Set();
+    }


Seems like you might want to turn some of this into helper functions

Sure. We can revisit next time on this.

wesm · 2019-02-27T23:31:55Z

r/src/array_from_vector.cpp

+      } else {
+        null_bitmap_writer.Set();
+        *p_indices = *p_factor - 1;
+      }


You might be able to write this without a branch

wesm · 2019-02-28T01:11:19Z

r/src/array_from_vector.cpp

+
+  virtual Status GetResult(std::shared_ptr<arrow::Array>* result) {
+    RETURN_NOT_OK(builder_->Finish(result));
+    return Status::OK();


you can return builder_->Finish(result)

Thanks. Will do in a next PR.

romainfrancois added Component: R ready-for-review WIP PR is work in progress and removed ready-for-review labels Feb 13, 2019

wesm force-pushed the master branch from 3088183 to 0c6b2d2 Compare February 18, 2019 19:34

romainfrancois force-pushed the ARROW-4560/array_no_dots branch 2 times, most recently from 0829631 to 916f76a Compare February 19, 2019 15:39

romainfrancois added ready-for-review and removed WIP PR is work in progress labels Feb 20, 2019

romainfrancois requested a review from wesm February 20, 2019 09:26

romainfrancois added 17 commits February 26, 2019 09:40

array() no longer take ...

762e000

prepare to handle the type argument in array()

86157bc

+ type() function to get or infer the arrow type

db05f36

converter/builder api for int types and boolean

26d1a95

handling array(type = float64())

5c9371d

handling array(type = float32())

2d31e2d

array(type = date32())

37b63ec

array(type = date64())

92c0819

array(type = timestamp())

712c22e

array(type = timestamp())

803570a

array(type = time32|64() )

0707f67

infer internally so that we know if we have infered, we need that for…

6d16fb6

… factors/dictionaries

cleanup

e23aabf

better format the error message

915d352

more tests about array(type = )

fd76173

no longer need remote vctrs because no longer use vec_c() internally …

25d0ac6

…since array() no longer deals with ...

relax factors

2bffd69

romainfrancois added 4 commits February 26, 2019 09:40

Simplify GetResult

541d069

implement ChunkedArray__from_list with single type inference so that …

ed0b973

…all arrays end up with the same type

tests for chunked_Array(type =)

9804fee

R package compatible with arrow 0.12

08b2953

romainfrancois force-pushed the ARROW-4560/array_no_dots branch from 4650fc0 to 08b2953 Compare February 26, 2019 08:40

wesm approved these changes Feb 27, 2019

View reviewed changes

wesm closed this in 2a14c7b Feb 27, 2019

wesm reviewed Feb 28, 2019

View reviewed changes

romainfrancois mentioned this pull request Mar 6, 2019

ARROW-3818: [R] Table$from_batches #3562

Closed

asfimport mentioned this pull request Mar 6, 2019

[R] array() needs to take single input, not ... #21107

Closed

ARROW-4560: [R] array() needs to take single input, not ... #3635

ARROW-4560: [R] array() needs to take single input, not ... #3635

Uh oh!

Conversation

romainfrancois commented Feb 13, 2019

Uh oh!

romainfrancois commented Feb 13, 2019

Uh oh!

romainfrancois commented Feb 14, 2019

Uh oh!

romainfrancois commented Feb 20, 2019

Uh oh!

wesm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wesm commented Feb 27, 2019

Uh oh!

wesm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants