JingsongLi commented on code in PR #10493:
URL: https://github.com/apache/paimon/pull/10493#discussion_r4253624547
##########
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 for investigating the tradeoff. I reran c77d425a: 166 related tests
passed, but the same historical Parquet files still produce 4 rows with the
baseline reader and 5 with this reader for both FLOAT and DOUBLE, exposing both
the old and new value for one primary key.
An explicit breaking upgrade with an offline migration is a viable
direction; this does not require keeping a permanent compatibility read path.
However, documentation alone leaves the confirmed silent-wrong-result path
reachable in an ordinary upgrade. Please make the rewrite prototype a runnable,
reviewable migration/preflight with the historical-file regression, and
explicitly reject or require an ordering-version/opt-in for unverified old
floating-key tables before scan planning. A table-level upgrade gate need not
scan every row on every read.
The migration must not read its input through the already-incompatible new
merge reader: rebuild from authoritative source data or correctly process the
physical runs, regenerate sorted files/bounds/manifests, stop old writers, and
document how historical snapshots/tags/branches and rollback are handled. A
controlled rebuild into a new table followed by validation and switch-over is
also a workable boundary. Until such a tested upgrade path is included, the
existing P1 remains unresolved for production rollout.
--
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]
