wangzhigang1999 commented on code in PR #10493:
URL: https://github.com/apache/paimon/pull/10493#discussion_r4245826311
##########
paimon-python/pypaimon/read/reader/sort_merge_reader.py:
##########
@@ -257,19 +245,10 @@ def comparator(row1: InternalRow, row2: InternalRow) ->
int:
raise ValueError(f"Unsupported {fields[idx].type} comparison")
if floating_flags[pos]:
- # Java Float/Double.compare: all NaNs tie above +inf, and
- # -0.0 precedes +0.0. Primary-key equality is unchanged.
- if math.isnan(val1):
- if not math.isnan(val2):
- return sign
- continue
- if math.isnan(val2):
- return -sign
- if val1 == val2 == 0.0:
- val1 = math.copysign(1.0, val1)
- val2 = math.copysign(1.0, val2)
-
- if val1 < val2:
+ result = compare_values(val1, val2)
Review Comment:
Thanks, Jingsong. I confirmed the upgrade regression and tried a
compatibility implementation. It adds substantial complexity across planning
and reading for this edge case. In a small local end-to-end test, even valid
floating-key files read about 17% slower.
Rejecting incompatible files before returning results also requires extra
validation; moving the check into heap merge would miss planning and raw-read
paths.
Given that tradeoff, would you accept an explicit breaking change requiring
affected users to rewrite their tables offline before upgrading? I validated a
rewrite prototype on small local Parquet cases, including your reproducer. I’d
prefer to keep this PR focused on correcting the semantics and document the
migration requirement, without adding permanent compatibility machinery to the
read path.
--
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]
