david-mollitor-db opened a new pull request, #59197:
URL: https://github.com/apache/spark/pull/59197
### What changes were proposed in this pull request?
`SizeEstimator.visitArray` estimates an object array with more than 400
elements from two samples of 100 elements and extrapolates the rest:
```scala
state.size += math.max(s1, s2) +
(size * ((length - ARRAY_SAMPLE_SIZE) / ARRAY_SAMPLE_SIZE))
```
`length` and `ARRAY_SAMPLE_SIZE` are both `Int`, so `(length -
ARRAY_SAMPLE_SIZE) / ARRAY_SAMPLE_SIZE` truncates before the multiplication and
up to 99 elements are left out of the estimate. This PR multiplies first, `size
* (length - ARRAY_SAMPLE_SIZE) / ARRAY_SAMPLE_SIZE`, which is evaluated in
`Long` because `size` is a `Long`.
### Why are the changes needed?
`MemoryStore` weighs a deserialized cached block with
`SizeEstimator.estimate` over the block's `Array[T]` of rows, so every block
whose row count is not a multiple of 100 is under-reported. For example, a
499-element array is estimated as if it had 400 elements. The truncation dates
back to SPARK-6738.
Measured against JOL's `GraphLayout.totalSize()` (itself checked against
`Instrumentation.getObjectSize`) on JDK 25 with Spark's executor `--add-opens`,
estimate / actual for one partition array of rows:
| Row shape | 450 rows, before | 450 rows, after | 499 rows, before | 499
rows, after |
|---|---|---|---|---|
| `Tuple3[Int, Long, Double]` | 0.894 | 1.000 | 0.810 | 1.000 |
| class with a `String` field | 0.888 | 0.994 | 0.809 | 0.999 |
| `Row(Long, String, Double, Int, Timestamp, Boolean)` | 0.893 | 1.002 |
0.805 | 0.998 |
| nested `List` / `Map` / `int[]` | 0.875 | 0.983 | 0.771 | 0.954 |
| `UnsafeRow` copies | 0.891 | 0.998 | 0.809 | 1.000 |
Lengths that are multiples of 100 are unaffected. The error is bounded by
99/N of the elements' size, so it matters most for arrays from 401 up to a few
thousand elements.
### Does this PR introduce _any_ user-facing change?
No API change. Size estimates for object arrays longer than 400 elements
whose length is not a multiple of 100 grow by up to 99 elements' worth, which
is closer to their actual footprint. This slightly changes memory accounting
for cached blocks and for size-tracked collections built on `SizeEstimator`
(e.g. spill thresholds).
### How was this patch tested?
- Added `sampled object arrays are extrapolated to their full length` to
`SizeEstimatorSuite`, asserting exact sizes for arrays of 401, 450, 499 and
1099 elements. Without the fix it fails with `Expected 11248, but got 11224`.
- Passed: `SizeEstimatorSuite`, `SizeTrackerSuite`, `PrimitiveVectorSuite`,
`OpenHashSetSuite`, `OpenHashMapSuite`, `MemoryStoreSuite` (core) and
`ApproximatePercentileSuite` (catalyst).
- The JOL comparison above was run with a local harness that is not part of
this PR.
### Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Claude Opus 5.5)
This pull request and its description were written by Isaac.
--
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]
