peter-toth commented on PR #58895: URL: https://github.com/apache/spark/pull/58895#issuecomment-5890470420
@dongjoon-hyun @cloud-fan thank you both. Round 4 is answered: every comment is either fixed in the code or answered with a reason in its thread, and the description is up to date with all of it. **Three of them changed the design rather than the code around it.** `parquet.filter.columnindex.enabled=false` no longer turns the feature off, it leaves the filter with the row groups it empties. A missing offset index is decided from the footer per row group, before anything is buffered, with parquet's exception kept as the backstop. And the missing-key rewrite is gone, so the row phase 1 evaluates takes the vectors `initBatch` built, which leaves one rule for what a missing column reads as. **There is a large refactor in this push, and I would like your read on it.** The read path is a subclass now, `LateMaterializationParquetRecordReader`, so `VectorizedParquetRecordReader` keeps the plain path and nothing else. Three things that are not about this feature stand on their own as well: a per-type `ValueCopier`, a `ParquetBlockAccounting` for the footer arithmetic, and the metric keys and labels on `StorageFilterMetrics`'s companion. My reason is reviewability. The single-file version was hard to read by hand, and the base class's diff against master is now five things with no plain-path logic among them, which can be checked on its own. It does change code you have already reviewed, so if you would rather have the shape you reviewed, say so and I will revert it, or move it into a PR of its own that lands before or after this one. The correctness fixes from this round are in separate commits, so a revert of the refactor does not take any of them with it. **The scattered-survivor measurement** is in the description. One long key and twenty long payload columns, 256-row pages, one survivor every other row: phase 2 skips no page at all and the read takes 5.6x the plain one, falling to 1.8x at runs of sixteen rows. So a gate is needed. It is a follow-up rather than part of this PR, because giving up also re-reads the key columns and hands the post-scan `Filter` every row the filter would have dropped, and those costs grow with what was excluded rather than with what was kept, so the direction flips as the filter gets more selective. **Two things came out of my own review rounds after yours.** The flag that says the page index cannot be used carried two meanings, and for the one a failed partial read latches, dropping `getRowRanges` for the rest of the file dropped the pushed data filter's own page pruning too. Measured at 215 rows where a plain read of the same query hands back 160, so the feature read more than not having it. There are two flags now, and a test on a hand-damaged file pins it, which is the first test to reach that retry at all. The other is the survivor budget, which charged a variable-length key 9 bytes per row where the vectors allocate 17, so a null-heavy string key could hold twice the cap. -- 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]
