dongjoon-hyun commented on PR #58596: URL: https://github.com/apache/spark/pull/58596#issuecomment-5581054961
The two changes here have quite different characters: the `ImageSchema` one looks good, but the `Interaction` one guards a path that I believe cannot be reached. ### 1. `ImageSchema.decode` — LGTM ```scala val imageSize = height.toLong * width * nChannels assert(imageSize < 1e9, "image is too large") val decoded = Array.ofDim[Byte](imageSize.toInt) ``` The widening is correct (`Long * Int * Int` stays `Long`, the `Long < Double` comparison is fine, and `toInt` is safe once the assertion has passed), and the overflow is genuinely reachable: a `BufferedImage` must have an `Int`-representable pixel count, but e.g. an 800M-pixel RGB image gives `8e8 * 3 = 2.4e9`, which wrapped negative, slipped past the assertion, and produced a `NegativeArraySizeException`. Now it fails with the intended "image is too large". I also checked that this is the only place in `mllib` doing an `nChannels` multiplication, so nothing is missed. ### 2. `Interaction` — the overflow this guards can't happen `size *= currentEncoder.outputSize` runs inside the UDF, i.e. on the executor. But before any of that, `transform` eagerly calls `getFeatureAttrs` on the driver: https://github.com/apache/spark/blob/master/mllib/src/main/scala/org/apache/spark/ml/feature/Interaction.scala#L75 and that method builds the full cross product of attributes via the nested `flatMap` at L174-L181, then `toArray` at L184 and `toMetadata()` at L116. The number of attributes it materializes is *exactly* the same product as `size` — for each input feature, the length of `encodedFeatureAttrs` equals that feature's `FeatureEncoder.outputSize`. So to get `size` anywhere near `Int.MaxValue`, the driver first has to materialize ~2 billion `Attribute` objects and serialize them into column metadata. It will OOM long before the UDF ever runs. There is no input that reaches the `Math.multiplyExact` call. A few consequences: - The PR description's failure mode — "wrapping to a bogus (possibly negative) size and producing corrupt indices" — can't actually occur, so as written the description records a bug that isn't there. - "How was this patch tested?" is right that the heap runs out first, but for a different reason: it's the driver-side attribute cross product, not the size of the input data. - If fail-fast on an oversized interaction is the goal, the check belongs on the driver right before L75 — computing the product of `featureEncoders.map(_.outputSize)` in `Long` and rejecting it there would fail with a clear message instead of OOM-ing while building billions of attributes. That would be a change users can actually observe. Also, even keeping the current placement, `Math.multiplyExact` throws `ArithmeticException("integer overflow")`, which then gets wrapped in `FAILED_EXECUTE_USER_DEFINED_FUNCTION`. That's not really the "clear arithmetic error" the description promises; a `require` with an explanatory message would read better. ### 3. Comment size Four lines of comment for a one-line change in `Interaction` (and three in `ImageSchema`) is heavy relative to the surrounding code. `Math.multiplyExact` / `.toLong` already state the intent; a single line on *why* overflow matters here should be enough. ### 4. Minor, out of scope `FeatureEncoder.outputSize = numFeatures.sum` can overflow `Int` in principle too, but only for an unrealistically large `numFeatures` array. Not worth touching in this PR. ### Summary | | | |---|---| | `ImageSchema.decode` widening | LGTM — correct, and fixes a reachable path | | `Interaction` `multiplyExact` | Please reconsider — unreachable as placed; either move the check to the driver at L75 or drop it from this PR | | PR description | The `Interaction` parts (corrupt indices / why it isn't testable) need correcting | | Comments | Suggest trimming | No build or lint concerns: types are correct, no non-ASCII, no lines over 100 chars. Dropping to just the `ImageSchema` change would be mergeable as is. -- 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]
