Uh oh!
There was an error while loading. Please reload this page.
ARROW-6310: [C++] IPC json should use strings for 64 bit ints - #5267
ARROW-6310: [C++] IPC json should use strings for 64 bit ints#5267bkietz wants to merge 4 commits into
Conversation
cdfe299 to
8318e9eComparepitrou
commented
Sep 10, 2019
You'll probably need to change the Java side as well (and perhaps Go). Maybe @emkornfield can help. |
wesm
left a comment
There was a problem hiding this comment.
We might need to set up an integration branch for this work
| class IntegerType(PrimitiveType): | ||
| class Column(PrimitiveColumn): |
There was a problem hiding this comment.
It isn't very idiomatic to declare nested classes, this should be IntegerColumn at the top level
| (std::is_same<UInt8Type, T>::value || std::is_same<Int8Type, T>::value); | ||
| }; | ||
| template <typename T, typename CType = void> |
There was a problem hiding this comment.
Should put this in json_internal.cc unless we anticipate using it elsewhere
There was a problem hiding this comment.
I do think IsSignedInt etc should be removed in favor of this trait or one like it (though it should probably be renamed).
IsSignedInt takes either an array or a type as a type argument, which is surprisingly atypical for traits. Furthermore whereas is_signed_integer returns false for date and other types which are represented by but not identical to integers IsSignedInt returns true by checking only the c_type, which leads to static_assert(IsSignedInt<HalfFloatType>::value, ""). Finally the declaration of IsSignedInt is far from readable due to nested macro usage
There was a problem hiding this comment.
wesm
commented
Sep 10, 2019
bkietz
commented
Sep 10, 2019
Java side seems to have been addressed already: #5002 |
wesm
left a comment
There was a problem hiding this comment.
+1. I changed the base to an integration branch where we can address the Go and JS side of this before merging into master
JS can't represent all 64 bit integers as numbers (which are doubles), so store them in the JSON IPC as strings instead. Closes#5267 from bkietz/6310-Write-64-bit-integers-as- and squashes the following commits: 09b6a94 <Benjamin Kietzman> de-nest IntegerColumn 8318e9e <Benjamin Kietzman> rewrite integration/integration_test.py to generate int64 cols as strings 121dee1 <Benjamin Kietzman> use std::to_string a5cd719 <Benjamin Kietzman> ipc json should use strings for 64 bit ints Authored-by: Benjamin Kietzman <bengilgit@gmail.com> Signed-off-by: Wes McKinney <wesm+git@apache.org>
Some 64 bit integer values are not representable as JSON numbers, so store these as strings. This is issue is a regression; original fix was #5267Closes#7292 from bkietz/8471-Regression-to-uint64-as-J Authored-by: Benjamin Kietzman <bengilgit@gmail.com> Signed-off-by: François Saint-Jacques <fsaintjacques@gmail.com>
JS can't represent all 64 bit integers as numbers (which are doubles), so store them in the JSON IPC as strings instead.