Add warning for float->string conversions. #212
Conversation
| @@ -21,6 +21,8 @@ Out of the existing `Spark SQL types <https://spark.apache.org/docs/latest/sql-r | |||
| - ``TimestampTime``, ``DateType`` | |||
| - ``ArrayType``, ``MapType`` | |||
|
|
|||
| *Warning:* Conversions from ``FloatType`` to ``StringType`` may produce incorrect results in the least significant portion of the floating point value. See `this issue <https://github.com/mc2-project/opaque/issues/211>`_ for further details. | |||
There was a problem hiding this comment.
I don't think "incorrect" is the right word to use here; after reading the issue, it seems that they're both the same float representation. Maybe something like "may produce a correct result rounded to a different decimal place than the one shown by FloatType"? But I also kind of don't think this warning is really necessary, since that should be pretty obvious when comparing show and collect results for example.
There was a problem hiding this comment.
Agreed that "incorrect" is the wrong word. Maybe the rephrase could be:
"Converting a FloatType to StringType may display a result rounded to a different decimal place than the one shown by FloatType even though the internal representations are identical."
I don't know if it's good to assume a user would run both show and collect. Even if they did, I it's very confusing if there was a discrepancy between the two. Thoughts @wzheng ?
There was a problem hiding this comment.
Another point to consider is that show only has "inconsistent behavior" when a user explicitly compares the results to Spark SQL. Opaque's behavior is correct as a standalone SQL engine. So I'd add this point as well to specify what we're comparing to.
Closes #211