taepper opened a new pull request, #50653:
URL: https://github.com/apache/arrow/pull/50653

   ### Rationale for this change
   
   This is part of the work for #35460. Back in January I started working on 
this change-set, but after discussions came up regarding the use of the 
`simdjson::dom` vs `simdjson::ondemand` front-end the development went stale. I 
saw that @Reranko05 started the work on #35460 and already made great progress, 
well done!
   
   I still wanted to put up this change-set as a possible view-point, but feel 
free to disregard @Reranko05 if you already started your own work.
   
   ### What changes are included in this PR?
   
   This changes `from_string.cc` to use RapidJSON instead of simdjson.
   ### Are there any user-facing changes?
   
   Yes, the API has changed to no longer allow non-utf8 compatible strings. 
`simdjson` does not support non-compliant inputs and does not intent to support 
it in the future 
[[1]](https://github.com/simdjson/simdjson/issues/1691#issuecomment-895286180) 
[[2]](https://github.com/simdjson/simdjson/blob/8e6bac94877f2d3d026000d36ce81e0aaf38d26f/doc/basics.md?plain=1#L3701)
 [[3]](https://github.com/simdjson/simdjson/discussions/2125)
   
   All call-sites in tests only used a transformation to json as an 
intermediate result to easily go from `string -> Array`, so I changed these to 
use different helper methods instead.
   
   Another difficult problem arises in the handling of `NaN` and `Inf` literals 
in inputs. `simdjson` follows the json standard, whereas `RapidJSON` 
implemented an extension where the following is a valid input:
   ```
   {"x": Inf, "y": -Inf, "z": NaN, "n": Infinity, "m": -Infinity}
   ```
   
   We need to decide whether to be breaking here or not. This PR now contains a 
non-breaking version of the change. This can be quite elegantly added by using 
the public `ondemand` api's function to retrieve the raw token string in the 
`ConvertNumber` function.
   
   Alternatively, we can wait for the next `simdjson` major release, which will 
ship with support for a `SIMDJSON_ENABLE_NAN_INF` compilation flag as per [this 
pull request](https://github.com/simdjson/simdjson/pull/2696). (this was only 
merged to simdjson main)
   
   ### Are these changes tested?
   
   Yes
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]

Reply via email to