peter-toth commented on PR #58895: URL: https://github.com/apache/spark/pull/58895#issuecomment-5836432465
Thank you both. @dongjoon-hyun's third round found the one path in this change that could return a wrong answer, and @cloud-fan's blocking comment landed on the same survivor loop from the other side. The change to look at first: `parquet.filter.columnindex.enabled=false` now turns this feature off entirely. Phase 0 honoured it, which was a half measure, because phase 2 reads part of a row group through the offset index whatever that conf says. A wrong index there pairs a row's key with another row's values, and the post-scan `Filter` cannot catch it, since the key it sees is the right one. The escape hatch is total now. The rest of the round: - One owner weighs the memory budget, after every surviving row, over the buffered key bytes and the row ranges together. A row group whose survivors fit in a single accumulator is weighed like any other, and the range half is far cheaper at the root: `ParquetReadState` coalesces lazily instead of materializing a list per column reader, so the `x leaves` factor is gone. - The reader fails open on an evaluation error instead of the planner rejecting cast keys, so an `int` joined with `bigint` is pushed again. - The phase-1 page store is closed at its own boundary, the key-slot pairing is read off the ordinals, the format is asked about the original expression rather than a canonicalized one, and the byte-metric walk reports an undercount instead of throwing, because `ignoreCorruptFiles` would turn a throw into a silently truncated file. - Tests for what was untested: a file with no offset index at all, written with the low-level writer overloads you named, and an AQE arm that asserts the final plan's scan still carries the filter. Three things are follow-ups in the description rather than in this PR: broadcasting the bloom bytes, letting a format declare its own metrics, and coarsening the survivor ranges instead of giving the filter up. One is a reasoned no, in its own thread: the offset-index footer preflight, which round 2 of this PR had already deleted for two reasons my new attempt had again. The description is restructured, with the phases and the six ways the reader stops short up front. The suite is at 99 tests. -- 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]
