HyukjinKwon commented on code in PR #59214:
URL: https://github.com/apache/spark/pull/59214#discussion_r4171795926
##########
python/pyspark/sql/variant_utils.py:
##########
@@ -413,10 +417,100 @@ 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`, so that
Review Comment:
**Nit (P3):** This comment, the `_float_to_json` docstring below, and the
test comment `The expected values are what the JVM to_json returns` state
parity with the JVM unconditionally. `Double.toString`/`Float.toString` only
became shortest-digit in JDK 19 (JDK-4511638), and Spark still runs on Java 17
(`docs/index.md`). I checked on Temurin 17 vs 25:
| value | JDK 17 `to_json` | JDK 19+ / this PR |
|---|---|---|
| double 2^-24 | `5.9604644775390625E-8` | `5.960464477539063E-8` |
| double 2^-44 | `5.6843418860808015E-14` | `5.684341886080802E-14` |
| float 2^-96 | `1.26217745E-29` | `1.2621775E-29` |
| double 2e23 | `1.9999999999999998E23` | `2.0E23` |
So on a Java 17 deployment, the new power-of-two test vectors are not what
`to_json` returns. The PR description already says this ("follows the
`Double.toString` specification of JDK 19 and later"), but that boundary isn't
recorded in the code. Someone who later compares against a Java 17 cluster
could "fix" the retry back to the longer strings. Could you mention JDK 19+ in
these three comments, e.g. "formats like JDK 19+ `Double.toString`; older JDKs
can print a longer, still round-tripping digit string for some values"? No
behavior change is needed.
**Verification:**
- **Inspection:** The helper block comment, the _float_to_json docstring and
the test expected-value comment qualify JVM parity with the JDK 19+
specification and mention the possible pre-JDK-19 divergence.
- **Compatibility:** All existing variant toJson vectors and
test_variant_type expectations still pass unchanged, showing the change is
text-only.
--
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]
