andygrove opened a new issue, #6045:
URL: https://github.com/apache/datafusion-comet/issues/6045
## Describe the bug
PR #6018 (`docs: split the PR review skill by area and correct the shuffle
contributor docs`) picked up
a review comment from @sunchao that I deferred rather than fixed in that PR,
since it is agent tooling
and contributor docs rather than production code. Filing it here so it does
not get lost.
Two places added by that PR say that native range partitioning rejects float
and double keys when
`spark.comet.exec.strictFloatingPoint` is enabled:
- `.ai/skills/review-comet-shuffle-pr/SKILL.md`, in the `RangePartitioning`
bullet under
"1. Which Implementation": "It also rejects float and double when
`spark.comet.exec.strictFloatingPoint`
is on."
- `docs/source/contributor-guide/native_shuffle.md`, in item 4 of the "When
Native Shuffle is Used" list:
"Float and double are also rejected when
`spark.comet.exec.strictFloatingPoint` is enabled."
That was true when I wrote it, but #5981 landed in the meantime (commit
c56cef77a) and it is no longer
true on main. `supportedRangePartitioningDataType` in
`CometShuffleExchangeExec.scala` now returns `true`
for `FloatType` and `DoubleType` with no strict-mode condition, because the
native range partitioner
normalizes its comparison keys and its sampled boundary rows the same way
the native sort does.
`CometSortOrder.getSupportLevel` matches, returning `Compatible()` for
scalar float and double regardless
of strict mode. `CometNativeShuffleSuite` has a test, "range partitioning on
floating-point uses native
shuffle when strictFloatingPoint=$strict", that runs for both settings with
`SortOrder.allowIncompatible=false` and expects a native exchange.
So as written, the skill tells a reviewer to flag a correct native plan as a
bug, and the contributor
guide documents a fallback that does not happen.
The rest of the restriction is still accurate. Range keys really are
primitive-only: nested types are
rejected because native cannot sort them, and collated strings because Comet
compares raw bytes. What
strict floating point still governs is floating-point values *nested* in
arrays, structs, or maps, which
are compared with Arrow's raw total ordering (issue #5507) — but those are
already rejected as range keys
for being nested, so it is not an additional condition at this level.
Original review comment:
https://github.com/apache/datafusion-comet/pull/6018#discussion_r4050482813
## Steps to reproduce
Read either sentence against `supportedRangePartitioningDataType` in
`spark/src/main/scala/org/apache/spark/sql/comet/execution/shuffle/CometShuffleExchangeExec.scala`
on main.
## Expected behavior
Both sentences describe the current gate: scalar float and double range keys
are supported in native
shuffle in strict mode as well as outside it, and the restriction covers
nested types and collated
strings only.
## Additional context
Depends on #6018 landing first, since both lines are added by it.
--
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]
