peter-toth commented on PR #58895: URL: https://github.com/apache/spark/pull/58895#issuecomment-6065406280
Thanks for the careful read, @viirya. Short answers below. **Where it pays off** - **The bloom has to exist first.** Right. #58635 adds a `RUNTIME_FILTER` hint that waives those heuristics, and it fits this feature well. A user who knows a join suits it can turn the conf on and hint that join. I tried it on top of this reader: on a broadcast join, which gets no bloom today, the hinted bloom let the fact scan skip 6 of its 10 row groups and return 100 of 100,000 rows. - **Physical locality.** Your model is right, but the page that counts is the payload column's, not the key's. The bytes are in the wide columns, and those hold few rows per page. The benchmark's 2 kB column holds about 375 rows in a default page, so 0.1% scattered survivors still skip 65 MB of about 100 MB there. A narrow column holds up to 20,000 rows, as you say, but it is also cheap to read. The description says the page size decides the saving. - **Delta and Iceberg.** The next two big steps we want to bring in are DSv2 support, which covers Iceberg, and per-partition bloom filters under SPJ. I'll also state the subclass exclusion in the conf doc. An explicit opt-in for subclasses is a good follow-up. - **Object storage.** Agreed, that is the measurement that matters, and we plan to run one. The offset indexes are mostly not extra. A plain read loads the offset index of every projected column whenever a data filter is pushed, and an inner join's key gets an inferred `isnotnull` that is pushed. So the extra cost is one more round of vectored requests per row group. That is lost where the filter prunes little. Where it prunes a lot, slow IO is where we expect the larger gain. - **The default.** A gating heuristic that gets this right for every workload is a hard problem, and that is why the conf is off by default. Until then the feature is for the workloads that clearly fit it, and the hint makes those easy to target. **Survivor buffer memory.** Agreed. It is in the follow-up list as a `MemoryConsumer` whose `spill()` gives splicing up, and I'll move it up. **Comments and the conf doc.** Agreed on both. I'll open a cleanup follow-up that keeps only the invariants and the non-obvious trade-offs, and your conf text is a good base for it. -- 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]
