Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Single-precision doubles always written at FLOAT64 instead of FLOAT32 #489

Open
mattbishop opened this issue Oct 29, 2018 · 3 comments
Open

Comments

@mattbishop
Copy link

A single-precision decimal value should be written as a smaller FLOAT32 instead of always using FLOAT64. A simple test for single-precision can be added to ImmutableDoubleValueImpl:

        public void writeTo(MessagePacker pk) throws IOException {
            // ValueFactory always writes a float64; needs to be a float32 for single-precision
            if (toDouble() == toFloat()) {
                pk.packFloat(toFloat());
            } else {
                pk.packDouble(toDouble());
            }
        }
@xerial
Copy link
Member

xerial commented Dec 2, 2018

@mattbishop It's because msgpack's specification of Float Value type:
https://github.com/msgpack/msgpack/blob/master/spec.md#type-system
This needs to be FLOAT64.

Value objects are implementation of MessagePack types systems, and it's a bit different MessagePack's format types, which have FLOAT32/64.

@mattbishop
Copy link
Author

The Float Value type decides how to encode itself into the msgpack stream, and the spec allows for either single-precision or double-precision float values in the encoded result. The single precision is shorter, and thus preferrable, when no data loss will occur.

float 32 | 11001010 | 0xca
-- | -- | --
float 64 | 11001011 | 0xcb

My request with this issue is to consider packing a double as Float32 when it will correctly encode the value, and thus save space. Msgpack is about saving bytes on the wire, after all.

@xerial
Copy link
Member

xerial commented Dec 11, 2018

@mattbishop I understand what you need. For clarity, msgpack has two specs for formats (with FLOAT32/64) and types (only FLOAT64).

  • (Double)Value is an implementation of msgpack types, so we need to follow the spec, that is, representing DoubleValues as double-precision values inside Value objects.

If we check float -> DoubleValue(double) -> float has no precision loss as you showed, it would be OK as long as DoubleValue.doubleValue() are correctly used in user applications. If some application assumes getting FLOAT64 msgpack type from DoubleValue, it will fail with this change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants