peter-toth commented on PR #58895: URL: https://github.com/apache/spark/pull/58895#issuecomment-5936075195
@dongjoon-hyun thank you for another careful round. Round 8 is answered in the threads, and the description is up to date. Several of these found real problems, and I fixed them together rather than one by one: - A fatal error now goes out as itself, found as deep as the executor would look had a plain plan raised it. A kill keeps its checked wrapper. - Every non-fatal error from the filter falls back, an `AssertionError` included. So does a codec `InternalError` while decoding a key page, which then fails at the same batch as a plain read. - A failure copying a survivor gives splicing up with a WARN of its own, and the per-type tests now fail on one. The reader's own invariants throw an internal error that `ignoreCorruptFiles` cannot take for a corrupt file. - A later accumulator set is sized up front only when its charge fits under the cap. - A `MissingOffsetIndexException` costs only its own row group. One more blind review round of ours after these found a few more, fixed in the same push: - An `InternalError` from the filter goes out in a checked wrapper, since `ignoreCorruptFiles` would take a bare one for a corrupt file. - A later set is sized up front only if that fits after the survivor's own charge. - Each kind of give-up WARN is reported once per split, rather than one for all of them. - New tests pin the metrics and the budget exactly. The branch is also rebased onto master. That brings SPARK-59828, without which a v2 file's `DELTA_BYTE_ARRAY` decimal column failed when survivors scatter. There is also a new `StorageFilterPushdownBenchmark`, in its own commit, as you asked in round 4 (https://github.com/apache/spark/pull/58895#discussion_r4108708299). It runs a join whose runtime bloom filter is the storage filter, with the feature off and on, over clustered keys and over keys scattered at 3%, 25% and 50%. The results files come from the GitHub Actions benchmark workflow. A few items repeat findings that earlier threads answered: - the 2-arg `initialize` override, which has to stay for MiMa (https://github.com/apache/spark/pull/58895#discussion_r4135661837); - the `ParquetReadState` shape, which the round-6 benchmark settled (https://github.com/apache/spark/pull/58895#discussion_r4137343688). And one goes against an earlier ask of yours. The second subclass check is the one you asked for in round 4 (https://github.com/apache/spark/pull/58895#discussion_r4108708266), and it is one private predicate now. Could the next round treat the settled ones as settled unless something new comes up? It would help keep the review on what is still open. -- 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]
