jverhoeks commented on issue #2376: URL: https://github.com/apache/iceberg-rust/issues/2376#issuecomment-4330331459
Update 2: tried a third follow-up that also got reverted. Cataloguing here so the next person doesn't burn a bench cycle on it. **Attempt 3: `OnceLock<Predicate>` first-success cache** To avoid the Mutex contention from attempt 2 and the conversion cost on every per-task call, I tried a `std::sync::OnceLock<Predicate>` that fills on the first call returning a non-None predicate and is read lock-free thereafter. The trade-off was acknowledged: we lock in the first snapshot we observe. **Why it failed in a new way:** multi-filter scans (TPC-H q04 / q06 / q08 / q09) have several `DynamicFilterPhysicalExpr` runtime filters that seal at *different times* as the upstream hash joins finish. The first task that observes ANY filter sealed converts to a Predicate that AND-combines whatever sealed so far. The OnceLock then locks that *partial* predicate in for the rest of the scan, so later tasks miss the additional pruning that would have come from filters sealing later. SF10: q04 +34%, q06 +74%, q16 +42% vs Path B-2 alone. Net total +5% vs Path B-2. **The composite lesson across all three attempts:** any cache-based optimisation here depends on knowing whether *all* dynamic filters in the scan's bundle are sealed before populating the cache. A single OnceLock keyed on "first non-None" is unsafe for multi-filter scans. A correct cache needs either: - per-filter `OnceLock` slots keyed on stable filter identity (which we don't have, because `current()` returns a fresh `Arc` from `remap_children`), or - a "this filter is sealed" predicate exposed by `DynamicFilterPhysicalExpr` so the cache can wait until all bundled filters have sealed before populating. The latter is the API ask: an `is_sealed()` (or equivalent monotonic state-completion signal) on `DynamicFilterPhysicalExpr` would let downstream consumers cache safely without observing the inner expression. Combined with the `generation`/`version` ask from the previous comment, that gives consumers everything they need: "has this changed since I last cached?" plus "is this final yet?". Branch is back at the clean Path B-2 implementation. Three failed attempts logged in our downstream feature doc; happy to share the doc as a gist if useful for the upstream discussion. -- 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]
