peter-toth opened a new pull request, #58523:
URL: https://github.com/apache/spark/pull/58523
### What changes were proposed in this pull request?
`InternalRowComparableWrapper` gains a public `orderingFor(dataTypes)` that
returns the ordering from its existing `orderingCache`, and the two
storage-partitioned join sites that built the same ordering by hand now go
through it.
`KeyedPartitioning.groupedKeyRowOrdering` called
`RowOrdering.createNaturalAscendingOrdering` directly, which is byte-for-byte
the `loadFunc` of that cache. `DataSourceV2ScanExecBase.outputPartitioning` did
the same to sort the partition keys it then hands to `KeyedPartitioning`.
### Why are the changes needed?
Two reasons, and the second is worth more than it looks.
**One definition instead of two.** `InternalRowComparableWrapper.equals` is
`ordering.compare(row, other.row) == 0` over the cached ordering, while
`groupedKeyRowOrdering` is what lays grouped partition keys out. So "two
partition keys are equal" and "two partition keys sort together" already had to
be the same relation, and they were, only by both call sites happening to name
the same function.
**The direct call is expensive.** Only the Janino step inside
`GenerateOrdering` is cached, keyed on the generated source. Everything
upstream re-runs per call: `ExpressionCanonicalizer` over each `SortOrder`,
building the whole Java source, `CodeFormatter.stripOverlappingComments`
rebuilding it line by line, then hashing the multi-KB body to probe the compile
cache. Measured in this worktree, for a two-column key:
RowOrdering.createNaturalAscendingOrdering 93-105 us per call
InternalRowComparableWrapper.orderingFor 0.16-0.6 us per call
`DataSourceV2ScanExecBase.outputPartitioning` is the site where that
repeats. It is a `def`, and an instrumented run of
`KeyGroupedPartitioningSuite` counted 31,922 calls across 1,167 scan instances,
a mean of 27 per scan. The other callers are once-per-something and get the
same ~100 us back as noise: `GroupPartitionsExec.groupAndSortByKeys` runs from
a `lazy val`, `EnsureRequirements` runs once per join per pass, and
`KeyedPartitioning.keyRowOrdering` is itself a `lazy val`.
Nothing gets slower. A `NonFateSharingCache` hit measured 0.163 us
single-threaded and 0.334 us with 16 threads on one key. The cached value is a
stateless comparator, so eviction costs a rebuild and nothing else, and almost
every type list the new callers look up is one the wrapper path already loads
for the same rows. The exception is `KeyedPartitioning.keyRowOrdering`, which
keys on `keyDataTypes`, and that falls back to the expression types when there
are no partition keys, so it can add an entry of its own.
`SortMergeJoinEvaluatorFactory` also calls `createNaturalAscendingOrdering`
directly and is deliberately left alone. Those are join-key rows from an
`UnsafeProjection`, never wrapped, and their counterpart is the child `Sort`'s
ordering rather than wrapper equality. They also run per partition on
executors, so feeding their schemas into this cache would evict partition-key
entries for no gain.
### Does this PR introduce _any_ user-facing change?
No. The cache's load function is the call that was being made directly, so
it produces the same ordering.
One caveat, pre-existing on the wrapper path and now extended to these two
sites. `createNaturalAscendingOrdering` goes through
`CodeGeneratorWithInterpretedFallback`, which reads
`spark.sql.codegen.factoryMode` on every call, while the cache reads it once
per type list for the JVM's lifetime. So a test that flips that internal conf
after an entry is loaded now gets the ordering built under the earlier mode.
The two orderings agree semantically, so this costs coverage rather than
correctness.
### How was this patch tested?
A new `InternalRowComparableWrapperSuite`, which the class did not have,
only a benchmark. It asserts that `groupedKeyRowOrdering` and a wrapper hold
one ordering instance, and fails if the delegation is reverted.
Identity is what the test asserts because identity is what changes. Both
sides were already built by the same function, so no comparison of rows can
tell the two apart. What one instance buys is that neither side can later be
given a definition the other does not have.
### Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Opus 5)
--
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]
