Closed ChrisMuir closed 4 years ago
Oh and also, here's a bit of info on the current state of the code in my repo jsonparse
:
There's only one exported function, from_json()
. Currently, it handles data types character
, logical
, numeric
, and integer
. It handles scalar values, vectors (single type rapidjson arrarys), and lists (mixed type rapidjson arrays).
It does not handle nested JSON data, so like this fails:
library(jsonparse)
json_str <- "{\"json_key\":{\"yeah\":123, \"blah\":456}}"
from_json(json_str)
I think that's worth building out and fixing.
Let me know if there's anything else in particular that you have in mind.
Thanks!
It might be better if I add you as a collaborator so you can contribute directly, and we can both update the same branch if necessary?
My fromjson
branch is pretty bare, so feel free to delete it and start again. I really didn't get far.
Cool, yeah that sounds good.
Just to be clear though, would you prefer I contribute via pull requests or just by pushing directly to the repo?
Happy for you to push to a repo, preferably a branch while it's being developed, then I'll merge it to master once we're happy etc etc.
Got it, sounds good.
Started working on solving for nested json earlier this evening, just pushed some initial fixes to my repo. Still working on json nested within arrays, but initial results look good.
json_str <- "{\"json_key\":{\"foo\":123, \"bar\":456}}"
from_json(json_str)
$json_key
$json_key$foo
[1] 123
$json_key$bar
[1] 456
json_str <- "{\"int_key\":8, \"json_key\":{\"foo\":123, \"bar\":{\"bash\":456}}}"
jsonparse::from_json(json_str)
$int_key
[1] 8
$json_key
$json_key$foo
[1] 123
$json_key$bar
$json_key$bar$bash
[1] 456
Going to bed for now, tomorrow I will start working on migrating my code over to jsonify.
Just made an initial commit to branch fromjson
, here.
I got function from_json()
up and running. The guts of the C++ functions are essentially just a copy/paste from my repo, with some changes made to try to match your repo's style choices and conventions. The C++ functions that are doing all of the work are in inst/include/from_json/from_json.hpp
. The C++ function is called via rcpp_from_json()
, in the file src/from_json.cpp
......I tried to keep the structure and organization similar to what you had built out for to_json()
. Also added an R wrapper function, with examples, and added a few tests in the testthat
dir (all tests are passing for me).
Couple of things to mention:
src/from_json.cpp
and from_json.R
(with the C++ code just commented out). devtools::check()
is failing for me, due to one of the examples in to_json
using F
instead of FALSE
. I'm using devtools v2.0.1. The CRAN checks look good though, so idk. Oh and I just looked at Travis, looks like this check error is what's causing it to fail.Here's my devtools output:
── R CMD check results ───────────────────────────────────────────────────────────────────────── jsonify 0.1.0 ────
Duration: 22.7s
❯ checking examples ... ERROR
Running examples in ‘jsonify-Ex.R’ failed
The error most likely occurred in:
> base::assign(".ptime", proc.time(), pos = "CheckExEnv")
> ### Name: to_json
> ### Title: To JSON
> ### Aliases: to_json to_json.data.frame to_json.numeric to_json.character
> ### to_json.integer to_json.logical to_json.complex to_json.Date
> ### to_json.POSIXct to_json.POSIXlt to_json.list to_json.default
>
> ### ** Examples
>
>
> to_json(1:3)
[1] "[1,2,3]"
attr(,"class")
[1] "json"
> to_json(letters[1:3])
[1] "[\"a\",\"b\",\"c\"]"
attr(,"class")
[1] "json"
> to_json(data.frame(x = 1:3, y = letters[1:3]))
[1] "[{\"x\":1,\"y\":1},{\"x\":2,\"y\":2},{\"x\":3,\"y\":3}]"
attr(,"class")
[1] "json"
> to_json(list(x = 1:3, y = list(z = letters[1:3])))
[1] "{\"x\":[1,2,3],\"y\":{\"z\":[\"a\",\"b\",\"c\"]}}"
attr(,"class")
[1] "json"
> to_json(seq(as.Date("2018-01-01"), as.Date("2018-01-05"), length.out = 5))
[1] "[17532.0,17533.0,17534.0,17535.0,17536.0]"
attr(,"class")
[1] "json"
> to_json(seq(as.Date("2018-01-01"), as.Date("2018-01-05"), length.out = 5), numeric_dates = F)
Error in to_json.Date(seq(as.Date("2018-01-01"), as.Date("2018-01-05"), :
F used instead of FALSE
Calls: to_json -> to_json.Date
Execution halted
1 error ✖ | 0 warnings ✔ | 0 notes ✔
Oh also, please feel free to modify any of the code I pushed (or ask me to make modifications). I'm excited to contribute, but the end of the day this is your repo 😃
yeah my devtools
was seriously out of date, so I wasn't getting that check()
error - sorted now (on fromjson branch)
Hey, so quick update on this, I've worked some on the fromjson
branch, and have gotten the functionality to a pretty good spot. I added more tests to test-from_json.R
, and those tests provide a pretty good snapshot of what from_json()
is currently able to handle. In addition to nested JSON, the function can now handle NA/null values, and JSON data that is missing keys.
I also added to the tests/benchmarks.R
file, comparing the performance of from_json()
to jsonlite::fromJSON()
.
At this point I think the biggest thing left to work on (as far as I know) is valid JSON objects that exist within an array as the value in a key-value pair......for example:
js <- "{\"key1\":8, \"key2\":[{\"foo\":123}, {\"bar\":456}]}"
jsonify::from_json(js)
#> $key1
#> [1] 8
#> $key2
#> NULL
I'll keep working on this, but just wanted to give a quick update.
looks good. And this example js <- "{\"key1\":8, \"key2\":[{\"foo\":123}, {\"bar\":456}]}"
is the reason I was putting off working on this !
We also need to make sure round-trips work. And keep in mind this un-box feature too.
Yeah so my goal is to make the function 100% compatible with boxed and unboxed data. As for round tripping data, from_json()
currently only returns lists and vectors....do you want to add the ability to return a data.frame as well (similar to jsonlite
)?
The last example I mentioned yesterday is now working properly:
js <- "{\"key1\":8, \"key2\":[{\"foo\":123}, {\"bar\":456}]}"
jsonify::from_json(js)
#> $key1
#> [1] 8
#> $key2
#> $key2[[1]]
#> $key2[[1]]$foo
#> [1] 123
#> $key2[[2]]
#> $key2[[2]]$bar
#> [1] 456
identical(jsonify::from_json(js), jsonlite::fromJSON(js, simplifyVector = FALSE))
#> TRUE
I had to add a very short header file to facilitate, inst/include/from_json/utils.hpp. It might make sense to add the contents of that new file to the existing utils.hpp
file (inst/include/jsonify/utils.hpp
) after this branch gets PR'ed into master...whatever you'd prefer though.
rebased fromjson
branch with master
to include the unbox
code
I've gone through this branch and it's looking good. I've made some slight adjustments to the structure (mainly inline
-ed the header functions).
Are there any other cases which need to be handled?
Also, this causes a crash
from_json("1")
It might need some checks like this before it parses the Document or Array, to account for these single values.
Cool, the inline
edits you made look good.
I've been meaning to go through the jsonlite::fromJSON()
tests to see if there are any cases that I haven't considered or that aren't yet covered. I'll update here when I've done that.
Thanks for bringing up the from_json("1")
crash....after looking into it, it seems like the issue is that the input "1"
is not being caught by the doc.HasParseError()
check, and then when the input hits the downstream code it crashes.
For example, this returns 0
:
int test_func(const char * input) {
rapidjson::Document doc;
doc.Parse(input);
// Make sure there were no parse errors
if(doc.HasParseError()) {
Rcpp::Rcerr << "parse error for json string: "<< input << std::endl;
Rcpp::stop("json parse error");
}
return 0;
}
test_func("1")
While this will crash:
int test_func(const char * input) {
rapidjson::Document doc;
doc.Parse(input);
// Make sure there were no parse errors
if(doc.HasParseError()) {
Rcpp::Rcerr << "parse error for json string: "<< input << std::endl;
Rcpp::stop("json parse error");
}
return doc[0].GetType();
}
test_func("1")
Ohhh yeah, just saw your last comment and commit....yeah using doc.IsInt()
and doc.IsDouble()
in if
statements seems to work.
yeah I think that's the way forward. I'm heading out for the day if you want to make the changes. Otherwise I'll do them when I get back.
Just pushed edits to handle scalar string and bool input values. Added new tests as well, so these all pass now:
expect_equal(from_json("1"), 1)
expect_equal(from_json("1.5"), 1.5)
expect_equal(from_json("\"a\""), "a")
expect_equal(from_json("true"), TRUE)
benchmarks well too (bearing in mind this is a small set)
library(microbenchmark)
microbenchmark(
jsonify = { lst <- jsonify::from_json( mapdeck::geojson ) }
, jsonlite = { lst <- jsonlite::fromJSON( mapdeck::geojson ) }
, times = 5
)
# Unit: milliseconds
# expr min lq mean median uq max neval
# jsonify 12.95391 13.63449 16.62252 15.8809 16.51467 24.1286 5
# jsonlite 382.74442 412.16154 460.81432 463.1058 469.07373 576.9861 5
regarding this question
do you want to add the ability to return a data.frame as well (similar to jsonlite)?
part of me thinks if each of the elements of the doc_to_list
are single vectors, all of the same length, we should return a data.frame
.
What are your thoughts?
In particular I think this round-trip should work
df <- data.frame(id = 1:2, val = c("a","b"), stringsAsFactors = F)
to_json( df )
# [{"id":1,"val":"a"},{"id":2,"val":"b"}]
js <- '[{"id":1,"val":"a"},{"id":2,"val":"b"}]'
res <- from_json( js )
as.data.frame( res )
# id val id.1 val.1
# 1 1 a 2 b
Yeah I agree, I think that makes good sense. I should have a chance to work on this sometime this week or this weekend.
Update on this, just pushed initial edits to the fromjson
branch for returning a data frame. Your example now works:
js <- '[{"id":1,"val":"a"},{"id":2,"val":"b"}]'
from_json( js )
#> id val
#> 1 1 a
#> 2 2 b
I've added arg simplifyDataFrame
, with the default value being TRUE
. For now I went with the same arg name as jsonlite
, but we can rename it if you'd like.
It will only return a data frame if the input JSON string is an array of JSON objects, all with the same names, and the names must have consistent data types. So like this round trip works:
x <- data.frame(
"col1" = c(1, 2),
"col2" = c(1L, 2L),
"col3" = c(TRUE, FALSE),
"col4" = c("cats", "dogs"),
stringsAsFactors = FALSE
)
from_json(to_json(x))
#> col1 col2 col3 col4
#> 1 1 1 TRUE cats
#> 2 2 2 FALSE dogs
But tweaking your example some will result in a returned list:
x <- '[{"id":"cats","val":"a"},{"id":2,"val":"b"}]'
from_json(x)
#> [[1]]
#> [[1]]$id
#> [1] "cats"
#>
#> [[1]]$val
#> [1] "a"
#>
#>
#> [[2]]
#> [[2]]$id
#> [1] 2
#>
#> [[2]]$val
#> [1] "b"
I also added a few new tests, and updating the timing results in the benchmark file.
Let me know if you had anything else in mind for this feature, thanks!
ran a quick benchmark and it's running fast!
What do you think should happen if the input names are the same, but they contain different types?
x <- '[{"id":"cats","val":"a"},{"id":"2","val":"b"}]'
from_json(x)
# id val
# 1 cats a
# 2 2 b
vs
x <- '[{"id":"cats","val":"a"},{"id":2,"val":"b"}]'
from_json(x)
# [[1]]
# [[1]]$id
# [1] "cats"
#
# [[1]]$val
# [1] "a"
#
#
# [[2]]
# [[2]]$id
# [1] 2
#
# [[2]]$val
# [1] "b"
Well we can either default back to returning a list, like the function does now. Or we can use the R rules of data type coercion to get the data into a data frame.....So your second example would return a data frame with the id
variable being a character vector.
Currently the code I added to return a data frame is all written in C++. I tried writing it in R and it didn't benchmark well....timings were about 1.5x slower than jsonlite
. Writing data coercion rules in C++ from scratch seems like a huge pain, but yeah it would be better to have your second example return a data frame (and if the user wants to preserve the data types of the input JSON, they can use simplifyDataFrame = FALSE
). With that in mind, I'll give this some thought and try to brainstorm different solutions.
Hi! This package is awesome. Small thing about this branch—NULL
is translated to {}
, but {}
is translated back to an empty named list:
(x <- jsonify::to_json(NULL))
#> [1] "{}"
#> attr(,"class")
#> [1] "json"
jsonify::from_json(x)
#> named list()
jsonlite does the same thing (and isn't going to change), but since it's early days here, it'd be nice if jsonify could do a perfect round-trip. Which is asking a lot, I appreciate.
Hi @alistaire47, glad you're enjoying the package! It would be easy to implement this......I don't have a strong lean on whether this is a good idea though. @SymbolixAU, what do you think?
at the moment I can't see a problem with implementing this.
Y'all are awesome.
Just pushed some fixes on the issue of getting from_json()
to return a data frame when there are different data types across the JSON objects of an input array.
Here's a few examples, these all are now capable of returning a data frame:
library(jsonify)
library(jsonlite)
x <- '[{"id":1,"val":"a"},{"id":2,"val":"b"}]'
from_json(x)
#> id val
#> 1 1 a
#> 2 2 b
identical(fromJSON(x), from_json(x))
2. Same names, differing data types
```r
x <- '[{"id":"cats","val":"a"},{"id":2,"val":"b"}]'
from_json(x)
#> id val
#> 1 cats a
#> 2 2 b
identical(fromJSON(x), from_json(x))
#> TRUE
x <- '[{"id":"cats","val":"a"},{"id":2,"blah":"b"}]'
from_json(x)
#> id val blah
#> 1 cats a <NA>
#> 2 2 <NA> b
identical(fromJSON(x), from_json(x))
Only issue I've seen so far is this roundtrip doesn't work:
```r
target <- data.frame("id" = c(1L, 2L), "val" = c("a", NA), "blah" = c(NA, "b"), stringsAsFactors = FALSE)
json_str <- jsonify::to_json(target)
identical(from_json(json_str), target)
#> FALSE
from_json()
ends up returning string literal "NA"
values, as opposed to actual NA
values. So I'm going to look into that.
Just pushed a fix for the empty JSON string input thing, in commit 49d14cf. Updated output:
library(jsonify)
from_json("{}")
#> NULL
from_json("[]")
#> NULL
"[]"
was also an issue, was causing RStudio to crash, so I fixed that up as well.
Thanks for bring this up @alistaire47 !
Just pushed edits to fix the string literal "NA"
issue. So this roundtrip now works properly:
target <- data.frame("id" = c(1L, 2L), "val" = c("a", NA), "blah" = c(NA, "b"), stringsAsFactors = FALSE)
json_str <- jsonify::to_json(target)
identical(from_json(json_str), target)
#> TRUE
So for now, I think that completes all of the edge cases that I had in the front of my mind (I'm sure there are others though). @SymbolixAU , are there any other cases you have in mind that are not currently working properly?
At this point, the from_json()
codebase could benefit from being cleaned up some and optimized, so I will work on that sometime soon. May not be until this weekend or next week though.
yeah no worries. In my mind I'm scheduling v0.2 release (including from_json()
) for Feb 2019, as I'm currently working on getting mapdeck
out by Christmas.
Cool sounds good. I'll keep working on this, to clean up the code and make sure there are no obvious edge cases I haven't considered. For the most part though, it feels close to being stable. I'll speak up if I run into any questions about how specific situations should be handled.
Tried to reorganize the from_json
code to conform to your updated structure within inst/include
, see this commit. It may still need to be tweaked some, but it's much closer than it was prior.
I tried to add all of the variables and functions from include/jsonify/from_json/from_json_utils.hpp
to your existing utils.hpp
file, but kept getting different linker errors and "Symbol not found" errors related to function parse_value()
.
The issue is that parse_value()
needs to be defined in a header file separate of from_json.hpp
, because it calls parse_array()
, which itself calls parse_value()
. Worked with for a couple hours, and tried a bunch of different configurations, and couldn't get it to work without errors.
If you work with this more and have questions for me on any of the from_json
files, just let me know.
I think the restructuring is good. I've rebased it with master again to keep it up to date and resolve any conflicts (there weren't any).
I'm now thinking about round-trips, where the from_json( to_json( l ) )
in this example doesn't get back to where we started.
l <- list( x = 1:3, df = data.frame( val = letters[1:3]))
l
$x
[1] 1 2 3
$df
val
1 a
2 b
3 c
js <- to_json( l )
from_json( js )
$x
[1] 1 2 3
$df
$df[[1]]
$df[[1]]$val
[1] 1
$df[[2]]
$df[[2]]$val
[1] 2
$df[[3]]
$df[[3]]$val
[1] 3
How feasible do you think it is to keep track of the size and type of each element as we parse down the JSON structure, and then force it to the best-fitting R structure?
Hey, sorry for the radio silence, still getting settled in from the move and new job. Looking at this now, I think it will be doable. Should have something this week, hopefully in the next few days.
Something which could trip you up is an example where a data.frame has a nested-list as a column
df <- data.frame( id = 1:2, val = I(list( x = 1:2, y = 3:6 ) ), val2 = I(list( a = "a", b = c("b","c") ) ) )
to_json( df )
# [{"id":1,"val":{"x":[1,2]},"val2":{"a":["a"]}},{"id":2,"val":{"y":[3,4,5,6]},"val2":{"b":["b","c"]}}]
Is it obvious how to get this back to the original data.frame
?
Note also this is handled differently to jsonlite
jsonlite::toJSON( df )
# [{"id":1,"val":[1,2],"val2":["a"],"_row":"x"},{"id":2,"val":[3,4,5,6],"val2":["b","c"],"_row":"y"}]
and if it helps, I've started to document how the mapping from R - to - JSON is implemented https://symbolixau.github.io/jsonify/articles/jsonify.html
I've merged the master branch into fromjson
to keep it up to date. However, it's caused this test to fail
target <- data.frame(matrix(nrow = 2, ncol = 2), stringsAsFactors = FALSE)
colnames(target) <- c("id", "val")
target[["id"]] <- list("a", NA)
target[["val"]] <- list(NA, c(1L, 2L, 3L, 4L))
json_str <- '[{"id":"a"},{"val":[1,2,3,4]}]'
expect_equal(from_json(json_str), target)
json_str <- jsonify::to_json(target)
expect_equal(from_json(json_str), target)
I'm hoping to get into the guts of all your work over the next few days/weeks, so I can have a go at fixing it if you don't manage to get to it before then.
I've been going through the fromjson
code in detail, and it all works very well, apart from the simplification. I don't think this is quite right. I'm going to continue working on it. Otherwise this is really good stuff.
Hey, yeah I'm sorry I kinda dropped off the face of the Earth. Between a new house, home renovations, and a new job, I've been too busy to do anything with R development recently. If you have any questions while working through it though, just let me know, I'd be happy to try to answer.
I think this is nearly done. On the debug branch I've restructured the code because it needed to be recursive. So I've made a json_to_sexp()
function, which is called recursively each time a JSON object {}
or []
array is found. All the logic of the various doc_to_list()
and parse_array()
etc functions are now inside here.
The branch is not ready for general use yet, but it's close.
I've also made a few decisions on how simplification will work, which deviate slightly from jsonlite
For example
js <- '[{"id":1,"val":1},{"id":2}]'
from_json( js )
# [[1]]
# [[1]]$id
# [1] 1
#
# [[1]]$val
# [1] 2
#
#
# [[2]]
# [[2]]$id
# [1] 1
Whereas if it were filled with NAs
jsonlite::fromJSON( js )
# id val
# 1 1 2
# 2 1 NA
But this doesn't work when converting back to JSON
to_json( jsonlite::fromJSON( js ) )
# [{"id":1,"val":2},{"id":1,"val":null}]
I want the round-trip to be consistent, and not have to handle edge-cases of inserting & removing null
/NA
values
to_json( from_json( js ), unbox = T )
# [{"id":1,"val":2},{"id":1}]
NULL
is consitentto_json( NULL )
# {}
from_json( '{}' )
# NULL
to_json( list() )
# []
from_json( '[]' )
# list()
There's just one simplify
argument. If TRUE
, where possible
3.1 arrays go to vectors 3.2 array of arrays go to matrices 3.2 objects go to data.frame
json_to_sexp()
function )data.frame
s are working correctly, and other edge-cases I find along the way (e.g. js <- '[[[1,1],3,[1,2]],[1,3],{},[{},{}]]'; from_json( js )
Rcpp::Rcout
callsby
argument is needed. - no. not doing.jsonlite
from suggests some benchmarks
jfy <- from_json( mapdeck::geojson )
jlt <- jsonlite::fromJSON( mapdeck::geojson )
library(microbenchmark)
microbenchmark(
jfy = {
jsonify::from_json( mapdeck::geojson )
},
jlt = {
jsonlite::fromJSON( mapdeck::geojson )
},
times = 2
)
# Unit: milliseconds
# expr min lq mean median uq max neval
# jfy 37.43178 37.43178 47.28685 47.28685 57.14192 57.14192 2
# jlt 364.72608 364.72608 384.98716 384.98716 405.24823 405.24823 2
x <- rnorm(1e6)
y <- rnorm(1e6)
z <- rnorm(1e6)
m <- matrix(rnorm(1e6), ncol = 2)
l <- sample(letters, 1e6, replace = T)
l <- list(x, m, list(y, list(z)), l)
js <- to_json( l )
jfy <- from_json( js )
jlt <- jsonlite::fromJSON( js )
str( jfy )
str( jlt )
library(microbenchmark)
microbenchmark(
jfy = {
jsonify::from_json( js )
},
jlt = {
jsonlite::fromJSON( js )
},
times = 2
)
# Unit: milliseconds
# expr min lq mean median uq max neval
# jfy 882.9696 882.9696 1013.968 1013.968 1144.966 1144.966 2
# jlt 2499.3025 2499.3025 2763.644 2763.644 3027.985 3027.985 2
library(microbenchmark)
ul <- "https://raw.githubusercontent.com/SymbolixAU/data/master/geojson/SA1_2016_ACT.json"
microbenchmark(
jsonlite = {
jlt <- jsonlite::fromJSON( ul )
},
jsonify = {
jfy <- jsonify::from_json( ul )
},
times = 5
)
# Unit: seconds
# expr min lq mean median uq max neval
# jsonlite 1.433624 1.486971 1.631894 1.537450 1.563000 2.138424 5
# jsonify 1.510242 1.528668 1.920362 1.531584 1.877338 3.153977 5
8386393 nice work. it may be worth making curl
an additional dependency.
@artemklevtsov how do you mean?
When input detected as URL check the curl
dependency and use it. We can see the same logic now in the jsonlite::fromJSON
.
Also I think than not all users need to work with URLs.
ok I understand - thanks.
We can use the base R functionality to work with URL when curl
is not installed or raise error when curl
is installed.
Hey, just wanted to formalize our twitter conversation in an issue.
I have built out a pkg that parses JSON data (jsonparse) that utilizes the rapidjson C++ library. Since I'm not planning on submitting it to CRAN, I'm happy to contribute the code to jsonify, and to help develop it further if need be.
I'll fork this repo and will plan on working in the
fromjson
branch. If there's anything I should know about the repo and/or branch, just let me know.