feat(spanner): add spanner::Value support for TypeCode::FLOAT32 #13862
Conversation
Represented as a `float` in C++, so no need for a wrapper class.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #13862 +/- ##
==========================================
+ Coverage 93.05% 93.65% +0.60%
==========================================
Files 2176 2263 +87
Lines 184982 195424 +10442
==========================================
+ Hits 172126 183015 +10889
+ Misses 12856 12409 -447 ☔ View full report in Codecov by Sentry. |
| @@ -535,6 +560,22 @@ StatusOr<std::int64_t> Value::GetValue(std::int64_t, | |||
| return x; | |||
| } | |||
|
|
|||
| StatusOr<float> Value::GetValue(float, google::protobuf::Value const& pv, | |||
| google::spanner::v1::Type const&) { | |||
| if (pv.kind_case() == google::protobuf::Value::kNumberValue) { | |||
There was a problem hiding this comment.
Do we need to worry about loss of precision or anything like that? If not, maybe a comment explaining why?
There was a problem hiding this comment.
I don't believe we have anything to worry about in practice. We do have the (perhaps less than enlightening "narrowing" comment, due to the decision upstream to re-use google.protobuf.Value { double number_value } for the wire format. We already assert that a C++ double can represent such a value, and now we also assert that float -> double [-> proto-double -> double] -> float is an unsurprising conversion path. If it is surprising (or if it can't even be done in a language), they prescribe using the native 64-bit floating-point type to represent FLOAT32.
There was a problem hiding this comment.
Ack. Just consider a more enlightening comment, I read it and thought "yes, it is narrowing, but why is it Okay in this case?" Would be nice if we could succinctly answer that question.
| } | ||
| std::string const& s = pv.string_value(); | ||
| auto const inf = std::numeric_limits<float>::infinity(); | ||
| if (s == "-Infinity") return -inf; |
There was a problem hiding this comment.
Just to show that I fear floating point, but I don't understand it: I think C++ does not guarantee that - std::numeric_limits<float>::infinity() yields a valid value, or that ::infinity() even exists. OTOH, on the platforms we could possibly care about, those things don't matter.
There was a problem hiding this comment.
We could try to (statically?) assert std::numeric_limits<float>::has_infinity() somewhere, or otherwise require IEEE 754 support. Otherwise, I believe we'll at least get HUGE_VALF from infinity(), and so use it for "Infinity" on such a platform.
There was a problem hiding this comment.
Optional: add the static assert. But it could get repetitive if we do it in each place we use that assumption, your call.
There was a problem hiding this comment.
Done (in a single place). Also added one for double.
Represented as a
floatin C++, so no need for a wrapper class.This change is