ARROW-8471: [C++][Integration] Represent 64 bit integers as strings #7292
Conversation
| @@ -26,7 +26,8 @@ class GoTester(Tester): | |||
| CONSUMER = True | |||
|
|
|||
| # FIXME(sbinet): revisit for Go modules | |||
| GOPATH = os.getenv('GOPATH', '~/go') | |||
| HOME = os.getenv('HOME', '~') | |||
There was a problem hiding this comment.
Why the change, just set GOPATH if you have something else?
There was a problem hiding this comment.
I don't know why ~ failed resolve to $HOME locally but this seemed like a way to save someone the next person from the frustration of reading the script to figure out that GOPATH needs set.
cpp/src/arrow/type_traits.h
Outdated
| template <typename T> | ||
| using is_physical_integer_type = | ||
| std::integral_constant<bool, is_physical_unsigned_integer_type<T>::value || | ||
| is_physical_signed_integer_type<T>::value>; |
There was a problem hiding this comment.
not sure 100%. but, is this indentation correct?
There was a problem hiding this comment.
C++ indentation is deferred to clang-format. We have a CI job which ensures that running clang-format is a no-op. As for explaining why it made this choice... I'm not familiar enough with its implementation to say, sorry
bkietz
force-pushed
the
8471-Regression-to-uint64-as-J
branch
from
9b581bb to
1ec2517
Compare
fsaintjacques
left a comment
fsaintjacques
left a comment
There was a problem hiding this comment.
Good thing we actually validates the extreme value now.
|
@bkietz one thing, you didn't change anything in Java, so either it was already behaving like this, or the test is disabled for it? If this is disabled, that should be a blocker for 1.0, parsing (u)int64 seems like a blocking requirement. |
Java probably accepts both forms, because AFAICT the test isn't disabled for it. |
|
I just looked at the Java code: for Int64 and UInt64, it uses |
|
@sbinet Could you validate the Go changes here? |
|
I say we move on and accept the go changes. |
|
This patch is failing some of the time in Appveyor due to integer narrowing. I'm fixing the problems in #7352 |
5 participants
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 #5267