HyukjinKwon commented on code in PR #59214:
URL: https://github.com/apache/spark/pull/59214#discussion_r4248728844


##########
python/pyspark/sql/variant_utils.py:
##########
@@ -413,10 +417,127 @@ def handle_array(value_pos_list: List[int]) -> str:
             if isinstance(value, bytes):
                 # decoding simply converts byte array to string
                 return '"' + base64.b64encode(value).decode("utf-8") + '"'
-            if isinstance(value, (datetime.date, datetime.datetime)):
+            if isinstance(value, float):
+                return cls._float_to_json(value, is_float32)
+            if isinstance(value, decimal.Decimal):
+                return cls._decimal_to_json(value)
+            if isinstance(value, datetime.datetime):
+                return '"' + cls._timestamp_to_json(value) + '"'
+            if isinstance(value, datetime.date):
                 return '"' + str(value) + '"'
             return str(value)
 
+    # The helpers below format scalars the same way as the JVM 
`Variant.toJson` on JDK 19+, so
+    # that `VariantVal.toJson` and the `to_json` SQL function produce the same 
output. On older
+    # JDKs, `to_json` can print a longer, still round-tripping digit string 
for some doubles and
+    # floats (see `_float_to_json`).
+
+    @classmethod
+    def _float_to_json(cls, f: float, is_float32: bool) -> str:
+        """
+        Formats a double or float like Java's `Double.toString` and 
`Float.toString` on JDK 19+,
+        which print the shortest digits (JDK-4511638). Older JDKs can print a 
longer, still
+        round-tripping digit string for some values, e.g. 2^-24 as 
5.9604644775390625E-8 instead
+        of 5.960464477539063E-8. Non-finite values are quoted because they are 
not valid JSON
+        numbers.
+        """
+        if math.isnan(f):
+            return '"NaN"'
+        if math.isinf(f):
+            return '"Infinity"' if f > 0 else '"-Infinity"'
+
+        def round_trips(s: str) -> bool:
+            x = float(s)
+            if not is_float32:
+                return x == f
+            try:
+                rounded = struct.unpack("<f", struct.pack("<f", x))[0]
+            except OverflowError:
+                return False
+            if rounded != x:
+                # Rounding the decimal to a double first can change which 
float32 it rounds to
+                # only when the double is exactly the midpoint between two 
float32 values. Every
+                # such midpoint is an exact double, so resolve that tie with 
the exact decimal.
+                bits = struct.unpack("<I", struct.pack("<f", abs(rounded)))[0]
+                bits = bits + 1 if abs(x) > abs(rounded) else bits - 1
+                other = math.copysign(struct.unpack("<f", struct.pack("<I", 
bits))[0], rounded)
+                midpoint = (rounded + other) / 2
+                if x == midpoint:
+                    exact, exact_midpoint = decimal.Decimal(s), 
decimal.Decimal(midpoint)

Review Comment:
   **Nit (P3):** `decimal.Decimal(midpoint)` converts a float through the 
implicit constructor, which consults the caller's thread-local decimal context, 
not the explicit `context` created below. If an application turns on Python's 
strict float/Decimal mode (`decimal.getcontext().traps[decimal.FloatOperation] 
= True`), `toJson()`/`str()` raise `decimal.FloatOperation` here instead of 
returning JSON.
   
   I checked this on the current head:
   
   - The test's own vectors `0x15AE43FE` and `0x15AE43FD` raise.
   - 12,609 of 20,000 sampled float32 values in [1e6, 1e9) raise, and none 
below 1e6 do.
   - Doubles never reach this branch.
   
   Before this PR, floats never touched `decimal`, so this exception path is 
new. Even with the trap off, the branch sets the `FloatOperation` flag on the 
caller's context.
   
   `decimal.Decimal.from_float(midpoint)` is an exact, explicit conversion that 
the trap allows. Swapping it in removed every exception and left the output 
unchanged on 100,000 samples. My earlier `decimal.Decimal(float(s))` suggestion 
had the same problem, sorry about that.
   
   **Verification:**
   - **Regression:** Under a caller context with the FloatOperation trap 
enabled, the float32 midpoint vectors and a float32 value in the millions 
format to their expected JDK 19+ strings instead of raising.
   - **Compatibility:** All existing JVM-format vectors and test_variant_type 
expectations still produce the same strings under the default and 
lowered-precision contexts.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Reply via email to