[jira] [Commented] (ARROW-1716) [Format/JSON] Use string integer value for Decimals in JSON

["ASF GitHub Bot (JIRA)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/ARROW-1716?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16233619#comment-16233619 ] 

ASF GitHub Bot commented on ARROW-1716:
---------------------------------------

wesm commented on a change in pull request #1267: ARROW-1716: [Format/JSON] Use string integer value for Decimals in JSON
URL: https://github.com/apache/arrow/pull/1267#discussion_r148177501
 
 

 ##########
 File path: cpp/src/arrow/ipc/json-internal.cc
 ##########
 @@ -1094,6 +1108,32 @@ class ArrayReader {
   }
 
   template <typename T>
+  typename std::enable_if<std::is_base_of<DecimalType, T>::value, Status>::type Visit(
+      const T& type) {
+    typename TypeTraits<T>::BuilderType builder(type_, pool_);
+
+    const auto& json_data = obj_->FindMember("DATA");
+    RETURN_NOT_ARRAY("DATA", json_data, *obj_);
+
+    const auto& json_data_arr = json_data->value.GetArray();
+
+    DCHECK_EQ(static_cast<int32_t>(json_data_arr.Size()), length_);
+
+    for (int i = 0; i < length_; ++i) {
+      if (!is_valid_[i]) {
+        RETURN_NOT_OK(builder.AppendNull());
+        continue;
+      }
+
+      const rj::Value& val = json_data_arr[i];
+      DCHECK(val.IsString()) << "JSON value is not a string";
+      const Decimal128 value(val.GetString());
 
 Review comment:
   Can this fail?

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


> [Format/JSON] Use string integer value for Decimals in JSON
> -----------------------------------------------------------
>
>                 Key: ARROW-1716
>                 URL: https://issues.apache.org/jira/browse/ARROW-1716
>             Project: Apache Arrow
>          Issue Type: Improvement
>          Components: C++, Java - Vectors
>    Affects Versions: 0.7.1
>            Reporter: Phillip Cloud
>            Assignee: Phillip Cloud
>            Priority: Major
>              Labels: pull-request-available
>             Fix For: 0.8.0
>
>
> Suprisingly, Java and C++ integration tests pass after ARROW-1588. This hides a bug, because we're writing decimal values as hex encoded bytes.
> C++ and Java compare that the bytes are the same, but because C++ is interpreting everything as little endian after ARROW-1588 and Java is big endian the numbers these bytes represent will be different in their respective systems.
> I propose that instead of encoding DecimaArray/DecimalVector values as hex encoded bytes, we store the integer as a string when writing Arrow DecimalArray/DecimalVector data to JSON. This will allow us to compare that the bytes have the same meaning in both systems.
> This requires a change to the way Arrow writes JSON.
> [~icexelloss] was extremely helpful in helping me get to the bottom of this.
> cc [~icexelloss] [~wesmckinn] [~jnadeau]



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)