dongjoon-hyun commented on PR #58895: URL: https://github.com/apache/spark/pull/58895#issuecomment-5798769693
Thank you for the quick update, @peter-toth. The review fixes addressed the previous round well. I left another round of inline comments on `b9981cda93b` ([review](https://github.com/apache/spark/pull/58895#pullrequestreview-5293839352)). Here is the summary. **Correctness (these seem worth addressing before merge)** - **Evaluation order:** the extracted bloom is now evaluated before the remaining post-scan conjuncts. With ANSI mode, a key like `CAST(s AS BIGINT)` from `InjectRuntimeFilter` can throw on rows that an earlier guard used to filter out, so a query that works with the conf off fails with it on. - **`ignoreCorruptFiles`:** the same evaluation errors now happen inside `FileScanRDD`'s catch, so they are treated as a corrupt file and the rest of the file is silently skipped. - **Phase-2 memory:** per-row `finalRanges` make each non-key column reader build its own `RowRange` list. That can reach hundreds of MB per task, and `maxSplicedRowGroupBytes` doesn't bound it. - **Misleading failure:** the per-block `ColumnIndexStore` is memoized over all columns, so a missing or unreadable offset index on one key column fails phase 2 with a "written without a page index" message. **Minor** - `bytesAvoidedByPageFiltering` over-reports, because `SQLMetric.add` drops negative values. - The error message embeds the full bloom as hex. - Metrics are not updated when an all-missing-key file is skipped. **Design / cleanup** - Since the runtime bloom is result-preserving, could it be an optional hint (fall back to the plain read) instead of must-honor with a new exception type? - The all-keys reader branch is unreachable in production, which contradicts the description. - Some things could be simplified: survivor-buffer memory accounting, reusing `RowToColumnConverter`, per-row `byte[]` copies, per-file bloom re-deserialization, the `FileFormat` default-builder recursion trap, and SQLConf placement. -- 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]
