ivoson commented on PR #56649: URL: https://github.com/apache/spark/pull/56649#issuecomment-4775314911
> **0 blocking, 1 non-blocking, 0 nits.** Clean, well-tested PR. > > ### Suggestions (1) > * `BatchEvalPythonUDTFExec` calls `getInputIterator` without passing its `pythonMetrics`, so `pythonPeakPickledBatchBytes` is never populated on the UDTF pickle path — yet it pickles through the same contiguous-allocation code with the same OOM risk and already mixes in `PythonSQLMetrics`. The byte _cap_ is reasonably scoped out of UDTF, but the peak metric is pure observability; wiring the existing `pythonMetrics` map would extend it for free. (Non-blocking, and the file isn't in this PR's diff — fine to defer.) > > ## Verification > Confirmed the default (cap-off, `-1`) path is byte-for-byte unchanged: `convertRow(_, None)` makes every `sizeAcc.foreach(...)` a no-op so the converted objects match the old inline convert, and `pickle.dump(o, baos); baos.toByteArray` produces the same bytes as the old `pickle.dumps(o)` (the `ByteArrayOutputStream(1024)` only changes initial capacity). The byte-cap path's size estimate cannot affect results — a batch always holds >=1 row, so estimate error only changes batch granularity, never output; every leaf accounts a positive size (the `toJava` catch-all routes unknown types to `addLeaf`), enforced by the per-type drift-guard test. Thanks, also passed `pythonMetrics` for `BatchEvalPythonUDTFExec`. -- 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]
