ARROW-1588: [C++/Format] Harden Decimal Format

[cpcloud]

No description provided.

[wesm]

Nice, thanks for doing this! @jacques-n can you review and if you agree with the representation let's open a JIRA to change the Java code to byte swap as appropriate to satisfy BigDecimal?

[wesm]

reinterpret_cast here -- can this change the bit pattern?

[cpcloud]

static_cast from one builtin integer type to another of the same byte size won't change the underlying bits.

The compiler will also fail to type check this because reinterpret_cast from one integer to another isn't allowed by the standard: http://en.cppreference.com/w/cpp/language/reinterpret_cast.

[cpcloud]

Just to clarify where this behavior is defined in the standard: http://en.cppreference.com/w/cpp/language/implicit_conversion#Integral_conversions

The specific language is this:

If the destination type is unsigned, the resulting value is the smallest unsigned value equal to the source value modulo 2 ** n
where n is the number of bits used to represent the destination type.
That is, depending on whether the destination type is wider or narrower, signed integers are sign-extended [footnote 1] or truncated and unsigned integers are zero-extended or truncated respectively.

Here's the footnote :)

This only applies if the arithmetic is two's complement which is only required for the exact-width integer types. Note, however, that at the moment all platforms with a C++ compiler use two's complement arithmetic

We're using exact-width integer types here, so all behavior here is well-defined and within the standard.

[wesm]

Gotcha. there is always *reinterpret_cast<T*>(&val) but this is helpful to know

[wesm]

+1, I will wait also for Jacques to review in case we need to expand the language to elaborate on the representation. I opened https://issues.apache.org/jira/browse/ARROW-1691 about changing the Java implementation

[wesm]

I'm surprised the integration tests pass. Shouldn't they not?

[cpcloud]

Yes, I'm not sure why they're passing. I'll take a look and get to the bottom of it

[wesm]

@jacques-n could you take a look at this? we still need to figure out what's going on with the integration tests

[cpcloud]

Just to clarify from ARROW-1716 the integration tests pass because the interpretation of decimal bytes isn't being checked in the integration tests, so the bytes compare equal but if we were to print the value as the underlying big integer or int128 in c++ and compare those they would be different.

[wesm]

I think we should merge this and then address the integration test issue and Java fixes in follow up patches. @jacques-n or @siddharthteotia can you please review?

[jacques-n]

LGTM +1