LuciferYang commented on PR #55932: URL: https://github.com/apache/spark/pull/55932#issuecomment-5434395850
One thing to settle before this merges: "Does this PR introduce any user-facing change? No" does not hold. The old loop did not spin forever on a truncated data region. `remaining` is an `int`, `skip` returns -1 once the stream is dry so it grows by one per iteration, wraps negative at 2^31, and the loop exits and `skipBinary` returns normally. As long as nothing is read from that page afterward, the query used to finish with correct rows, after roughly 2^31 iterations per short value in a pure-CPU loop that `killTask` cannot interrupt. With `skipFully`, the same file now throws `ParquetDecodingException`, and under `ignoreCorruptFiles=true` `FileScanRDD` skips the rest of the file, so the query returns fewer rows than before. The new behavior is the right one; it just needs to appear in the description, together with a note that master already spins today and only exits because of the `int` wrap. The description currently files that spin under "issues a naive bulk form would introduce", which is true of the naive `long` version but reads as though master were fine. The classification is worth settling too, since as a sub-task of the SPARK-57415 performance umbrella with a title about the per-value skip, this will not reach the maintenance branches. -- 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]
