[SPARK-59505][PYTHON][CONNECT] Round HALF_UP when rescaling decimals in LocalDataToArrowConversion - #58797
[SPARK-59505][PYTHON][CONNECT] Round HALF_UP when rescaling decimals in LocalDataToArrowConversion#58797NestDream wants to merge 1 commit into
Conversation
d142c31 to
d6d8464
Compare
HyukjinKwon
left a comment
HyukjinKwon
left a comment
There was a problem hiding this comment.
0 blocking, 0 non-blocking, 0 nits.
The fix is correct, minimal, and well-scoped. value.quantize(exp, context=ctx) now rounds HALF_UP, matching JVM BigDecimal semantics (away from zero on ties, including negatives, which the tests cover with -1.005); only the rounding mode changed, so the SPARK-53938 bounded-precision context that avoided InvalidOperation on 38-digit values is preserved. The test coverage is thorough and cross-checks directly against CAST. Looks good to merge from a code standpoint; the only open point is the migration-note version/section, which the author already raised.
Verification
Static review; no build/tests run by me. Traced the converter (conversion.py:_create_converter decimal branch and LocalDataToArrowConversion.convert), confirmed quantize uses the new context, and reviewed all six changed test/doc files. The three deterministic scanners (text_quality, contract_claim_verifier, local_efficiency) reported NO FINDINGS; text_quality independently confirmed Decimal.set/Decimal.changePrecision use ROUND_HALF_UP in sql/api/.../types/Decimal.scala, corroborating the added comment.
PR metadata suggestions
- Migration-guide placement: the note is under 'Upgrading from PySpark 4.3 to 5.0' and says 'In Spark 5.0'. Since this is a wrong-result fix in released 4.1.x/4.2.x that you propose backporting, if it lands in branch-4.x (4.3) the note should move to the 'Upgrading from PySpark 4.2 to 4.3' section and read 'In Spark 4.3'. (You already called this out; flagging so the committer sets it to the final fix version.)
| Upgrading PySpark | ||
| ================== | ||
|
|
||
| Upgrading from PySpark 4.3 to 5.0 |
There was a problem hiding this comment.
I think we will have 4.4. and I personally think we don't need such a notification here.
There was a problem hiding this comment.
That makes sense, I've removed it
…in LocalDataToArrowConversion LocalDataToArrowConversion rescales a Decimal to the declared scale with a decimal.Context whose rounding is ROUND_HALF_EVEN, Python's default. The JVM rounds HALF_UP whenever it rescales a decimal (Decimal.set, Decimal.changePrecision, CAST), and the pickled Python UDF and UDTF paths and Classic createDataFrame inherit that because the JVM does the rescale for them. So an Arrow-optimized Python UDF, an Arrow UDTF, a Python data source or Spark Connect's createDataFrame turned 1.005 into 1.00 where SQL and Classic give 1.01, and Connect and Classic disagreed on the same createDataFrame input. The context now uses ROUND_HALF_UP. Tests cover the converter, the Arrow UDF, the Arrow UDTF, a Python data source and createDataFrame (Classic and Connect) with values that sit exactly on a rounding tie.
d6d8464 to
579bbd0
Compare
3 participants
What changes were proposed in this pull request?
LocalDataToArrowConversionrescales a PythonDecimalto the declared scale with adecimal.Contextwhose rounding mode isROUND_HALF_EVEN, Python's default. This PR changes it toROUND_HALF_UP, which is what the JVM uses whenever it rescales a decimal (Decimal.set,Decimal.changePrecision,CASTin ANSI and non-ANSI mode), and therefore what the pickled Python UDF and UDTF paths and ClassiccreateDataFrameproduce, since the JVM does the rescale for them. Tests are added for the converter, the Arrow-optimized Python UDF, the Arrow Python UDTF, a Python data source andcreateDataFrame, with values that sit exactly on a rounding tie, next to thetest_decimal_roundtests that SPARK-53938 added.The pandas-based paths (
spark.sql.legacy.execution.pythonUDF.pandas.conversion.enabled, scalarpandas_udf) do not use this converter; they hand the values to pyarrow casts, which raise on such values with the defaultspark.sql.execution.pandas.convertToArrowArraySafelyand truncate with it off. They are unchanged here.Why are the changes needed?
With the default Arrow-optimized Python UDF,
udf(lambda v: Decimal(v), DecimalType(20, 2))returns 1.00 for'1.005', 1.02 for'1.025'and 0.12 for'0.125', whileCAST(v AS DECIMAL(20, 2))and the same UDF withuseArrow=Falsereturn 1.01, 1.03 and 0.13. Spark Connect'screateDataFramegoes through the same converter, socreateDataFrame([(Decimal("1.005"),)], "d decimal(20, 2)")gives 1.00 in a Connect session and 1.01 in Classic. Arrow UDTFs and Python data sources use the converter as well. The rounding was introduced by SPARK-53938 (#52637), whose goal was to match Classic; the explicitContextarrived there to fix anInvalidOperationon 38-digit values (#52637 (comment)) and the rounding mode itself was not discussed. It shipped in 4.1.0 and is in 4.2.0 and 4.3.0-rc1.I reproduced this against a distribution built from unmodified master and against released 4.2.0, in Classic and Connect sessions, with pandas 3.0.5 and pyarrow 25.0.1. With this change every path returns 1.01, 1.03 and 0.13, the same as
CAST.Does this PR introduce any user-facing change?
Yes. A decimal returned by an Arrow-optimized Python UDF, an Arrow UDTF or a Python data source, or passed to
createDataFramethrough Spark Connect, that has more fractional digits than the declared scale and sits exactly on a tie now rounds HALF_UP likeCASTdoes, instead of HALF_EVEN. Values that are not on a tie are unchanged. Since this is a wrong-result fix in released 4.1.x and 4.2.x, I would propose backporting it to the maintenance branches.How was this patch tested?
New tests:
test_decimal_rescale_rounds_half_upintest_conversion.py;test_decimal_round_half_upintest_arrow_python_udf.py(three Classic suites and their three Connect parity suites),test_udtf.py(Classic and Connect parity),test_python_datasource.py(Classic and Connect parity) andtest_creation.py(Classic and Connect parity). On master every one of them fails with the HALF_EVEN values except the ClassiccreateDataFrametest, which passes on master because Classic rescales on the JVM and stays green. The modulestest_conversion,test_arrow_python_udf,test_creation,test_parity_creation,test_udf,test_udtfandtest_python_datasourcepass with the change (the twotest_udf_with_input_file_nametests intest_udfandtest_arrow_python_udfread a path relative to the source tree and were run from there). The reproduction script from the ticket was run against released 4.2.0 and against a master build with and without the change, in Classic and Connect sessions.Was this patch authored or co-authored using generative AI tooling?
Authored by Li Guo, assisted by Claude Code (Fable 5.1).