HyukjinKwon commented on code in PR #59190:
URL: https://github.com/apache/spark/pull/59190#discussion_r4161618691
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/window/WindowFunctionFrame.scala:
##########
@@ -489,7 +489,10 @@ final class SlidingWindowFunctionFrame(
// Only recalculate and update when the buffer changes.
if (processor != null && bufferUpdated) {
- processor.initialize(input.length)
+ // The window driver rejects partitions larger than `Int.MaxValue` rows
for this frame
+ // (see `supportsLargePartition`); `toIntExact` is a defensive backstop
that fails loudly
Review Comment:
**Nit (P3):** `supportsLargePartition` no longer exists. The latest commit
removed it and made `WindowEvaluatorFactoryBase.checkPartitionSizeLimit` reject
every partition over Int.MaxValue rows, whatever frames are active. This
comment, the matching ones in `UnboundedWindowFunctionFrame.prepare`,
`UnboundedPrecedingWindowFunctionFrame.prepare`, and
`UnboundedFollowingWindowFunctionFrame.write`, and the one in
`WindowSegmentTree.build` (`WindowFunctionFrame.supportsLargePartition`) send
readers to a missing member and suggest a per-frame opt-out. Could you point
them at `checkPartitionSizeLimit` and drop the 'for this frame' wording? For
example: `// The window driver rejects partitions larger than Int.MaxValue rows
(see WindowEvaluatorFactoryBase.checkPartitionSizeLimit); toIntExact is a
defensive backstop ...`
**Verification:**
- **Inspection:** A repository search for supportsLargePartition finds no
matches, and each rewritten comment names the actual driver-level guard.
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/ExternalAppendOnlyUnsafeRowArraySuite.scala:
##########
@@ -116,6 +116,20 @@ class ExternalAppendOnlyUnsafeRowArraySuite extends
SparkFunSuite with LocalSpar
assert(getNumBytesSpilled > 0)
}
+ test("length is tracked as a Long") {
Review Comment:
**Nit (P3):** This test still passes if `length` and `numRows` are reverted
to `Int`. `val emptyLen: Long = array.length` compiles through implicit
Int-to-Long widening, and 0 and 15 compare equal to 0L and 15L either way. The
suite already asserts small `length` values (for example, `assert(array.length
== inMemoryThreshold)`). So this test adds no protection against the wraparound
it describes, while appearing to. I'd drop it. If you want to keep a signal for
the widening, check the type directly, for example
`classOf[ExternalAppendOnlyUnsafeRowArray].getMethod("length").getReturnType ==
java.lang.Long.TYPE`.
**Verification:**
- **Regression:** The remaining ExternalAppendOnlyUnsafeRowArray tests pass,
and none of them claims Long-width coverage it cannot detect.
--
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]
