GH-34436: [R] Bindings for JSON Dataset

[thisisnic]

• Closes: [R] Bindings for JSON Dataset #34436

[github-actions]

• Closes: [R] Bindings for JSON Dataset #34436

[westonpace]

The C++ / bindings changes look good to me. I didn't look too much at the R part.

[paleolimbot]

This is so close!

I know that we don't necessarily have a way to extract options that we set on a Dataset; however, I think we should add a test with an open_dataset() that passes some though (i.e., use_threads and/or newlines_in_values and/or schema), and if possible, test that they actually did something.

For use_threads = FALSE: I believe that option_use_threads() returns FALSE on Windows, so this is sort of implicitly tested; however, I think at least one test should explicitly attempt to pass it.

For newlines_in_values = TRUE: You could do something like write {"key": "\nvalue"}\n to a file and attempt collecting to make sure the newline in the value comes through.

For schema: You could do something like write {"key": "value"}\n to a file and attempt collecting with schema(key = int32()) and make sure it errors?

[westonpace]

Just a few minor questions but this seems fine to me otherwise.

[westonpace]

It seems odd these two things are related. Am I missing something?

[thisisnic]

I must have copied and pasted this to the wrong place, good catch!

[thisisnic]

This is so close!

I know that we don't necessarily have a way to extract options that we set on a Dataset; however, I think we should add a test with an open_dataset() that passes some though (i.e., use_threads and/or newlines_in_values and/or schema), and if possible, test that they actually did something.

Actually, I'm going to push back on this. We don't test these options fully when reading CSV datasets, and given they relate to internal mechanisms of how things run not what is returned, it's going to be difficult to test here, and should be tested in the C++ anyway. Happy to write tests to check that these options are passed through successfully though.

[paleolimbot]

I don't think that our previous lack of of test coverage should mean that we continue to add features that are not tested. I am also happy to defer to another review on this, though.

How about:

library(arrow, warn.conflicts = FALSE)
#> Some features are not enabled in this build of Arrow. Run `arrow_info()` for more information.
library(testthat, warn.conflicts = FALSE)

test_that("JsonParseOptions are passed through to the dataset", {
  tf <- tempfile()
  on.exit(unlink(tf))
  write('{"key": "value"}', tf)
  
  expect_error(
    # (but instead use open_dataset())
    read_json_arrow(tf, schema = schema(key = int32())),
    "parse error"
  )
})
#> Test passed 😀

(Ideally we'd be able to do something like dataset$parse_options$schema instead, which I think you've started to implement with the CSV options, but this seems slightly easier)

[paleolimbot]

and should be tested in the C++ anyway.

That is a good point...we're definitely not here to test C++; however, we do need to check that the wires are plugged in where it's reasonable to do and I think here it is reasonable to do.

[thisisnic]

I don't think that our previous lack of of test coverage should mean that we continue to add features that are not tested. I am also happy to defer to another review on this, though.

How about:

library(arrow, warn.conflicts = FALSE)
#> Some features are not enabled in this build of Arrow. Run `arrow_info()` for more information.
library(testthat, warn.conflicts = FALSE)

test_that("JsonParseOptions are passed through to the dataset", {
  tf <- tempfile()
  on.exit(unlink(tf))
  write('{"key": "value"}', tf)
  
  expect_error(
    # (but instead use open_dataset())
    read_json_arrow(tf, schema = schema(key = int32())),
    "parse error"
  )
})
#> Test passed 😀

(Ideally we'd be able to do something like dataset$parse_options$schema instead, which I think you've started to implement with the CSV options, but this seems slightly easier)

Feel free to push to the branch @paleolimbot

[paleolimbot]

Let's track separately! (See #36138)

[conbench-apache-arrow]

Conbench analyzed the 6 benchmark runs on commit 5ecdd945.

There were 4 benchmark results indicating a performance regression:

• Commit Run on arm64-t4g-linux-compute at 2023-06-18 22:56:25Z

  • params=threads:4/task_cost:100000/real_time, source=cpp-micro, suite=arrow-thread-pool-benchmark

• Commit Run on ursa-thinkcentre-m75q at 2023-06-19 01:28:10Z

  • params=<Subtract, UInt64Type>/size:524288/inverse_null_proportion:0, source=cpp-micro, suite=arrow-compute-scalar-arithmetic-benchmark

• and 2 more (see the report linked below)

The full Conbench report has more details.