JingsongLi commented on code in PR #10493:
URL: https://github.com/apache/paimon/pull/10493#discussion_r4244761712
##########
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:
[P1] Handle historical sorted runs before changing their key order
This comparator also reads existing Python files, whose rows were sorted
with signed zeros tied. I reproduced an upgrade using a historical run
`(-1,'z','low'), (+0,'a','positive-a'), (-0,'b','negative-old'),
(1,'z','high')`, then a new one-row commit `(-0,'b','negative-new')`. The
finite sentinel keys give valid min/max bounds and both files enter the same
merge section. On these identical committed files, the baseline reader returns
4 rows and only `negative-new`; this PR returns 5 rows, exposing both
`negative-new` and `negative-old` for the same primary key. This happens with
both FLOAT and DOUBLE because the old iterator is no longer sorted under the
comparator required by the heap merge.
The compatibility note acknowledges rewrites, but there is no ordering
version, migration gate, or validation preventing this silent wrong result
during an ordinary upgrade. Please provide a tested compatibility/rewrite path,
or reject incompatible historical runs explicitly before merging them under the
new order. Add this historical-file upgrade regression alongside the new-write
tests.
--
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]
