andygrove opened a new pull request, #6049:
URL: https://github.com/apache/datafusion-comet/pull/6049
## Which issue does this PR close?
Closes #6045.
## Rationale for this change
#6018 added two statements saying that native range partitioning rejects
float and double keys when
`spark.comet.exec.strictFloatingPoint` is enabled. That was true when it was
written, but #5981
landed in the meantime 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.
So as written, the skill told a reviewer to flag a correct native plan as a
bug, and the contributor
guide documented a fallback that does not happen.
The rest of the restriction is still accurate and is left alone. 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 (#5507), but those are already rejected
as range keys for being
nested, so it is not an additional condition at this level.
## What changes are included in this PR?
Two prose corrections, no code:
- `.ai/skills/review-comet-shuffle-pr/SKILL.md`, the `RangePartitioning`
bullet under "1. Which
Implementation". Replaces the strict-mode sentence with the current gate,
notes that
`CometNativeShuffleSuite` pins it under both settings of the config, and
extends the "not
automatically a bug" line after the two bullets so it covers the float
range key as well as the
struct or array hash key.
- `docs/source/contributor-guide/native_shuffle.md`, item 4 of the "When
Native Shuffle is Used"
list. Same correction, in that doc's voice.
The user-guide docs (`tuning.md` and `compatibility/floating-point.md`)
already describe the current
behavior correctly, since #5981 updated them, so they are untouched.
## How are these changes tested?
Documentation only, so there is nothing new to test. The behavior the text
now describes is already
covered by `CometNativeShuffleSuite`'s "range partitioning on floating-point
uses native shuffle
when strictFloatingPoint=$strict", which runs for both settings with
`SortOrder.allowIncompatible=false`
and expects a native exchange. `npx prettier --check` passes on both files.
--
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]
