Yicong-Huang commented on code in PR #57911:
URL: https://github.com/apache/spark/pull/57911#discussion_r3826187725
##########
python/pyspark/sql/conversion.py:
##########
@@ -804,13 +831,19 @@ def convert_string(value: Any) -> Any:
if not nullable:
raise PySparkValueError(f"input for {dataType} must
not be None")
return None
+ elif type(value) is str:
Review Comment:
Measured it. When it's really a str, isinstance and `type(v) is str` are the
same speed, but on a miss isinstance is ~17ns slower (44 vs 27ns), and that
hits every element on a coercion-heavy column, pushing all-coercion
`array<string>` from ~+9% to ~+21%. And `type(v) is str` is already correct for
`np.str_`: it falls through to `element_conv` -> `str(value)`, so we don't lose
anything, we just don't fast-path a rare input. Keeping the exact check.
##########
python/pyspark/sql/conversion.py:
##########
@@ -742,6 +763,12 @@ def convert_binary(value: Any) -> Any:
if not nullable:
raise PySparkValueError(f"input for {dataType} must
not be None")
return None
+ elif type(value) is bytes:
Review Comment:
Same as the string case: same speed on a hit, ~17ns slower per miss.
`np.bytes_` already converts correctly through `element_conv`, so keeping
`type(v) is bytes` and the `(bytes, bytearray)` assert.
##########
python/pyspark/sql/conversion.py:
##########
@@ -804,13 +831,19 @@ def convert_string(value: Any) -> Any:
if not nullable:
raise PySparkValueError(f"input for {dataType} must
not be None")
return None
+ elif type(value) is str:
+ # Fast path: `str(value)` returns `value` itself for a
`str`
+ # input (no copy), but still pays the constructor dispatch
per
+ # element. Returning it directly skips that and the bool
check.
+ return value
+ elif value is True:
+ # To match the PySpark Classic which convert bool to
string in
+ # the JVM side (python.EvaluatePython.makeFromJava)
+ return "true"
+ elif value is False:
+ return "false"
Review Comment:
`bool` can't be subclassed in Python, so `value is True`/`is False` already
covers every possible bool and isinstance would be identical. `np.bool_` isn't
a bool subclass either, so neither catches it (yields "True", same as master,
separate from this change). Keeping as is.
--
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]
