andygrove commented on issue #4988: URL: https://github.com/apache/datafusion-comet/issues/4988#issuecomment-5035038588
Follow-up measurements. **Items 2 and 3 in the description above are superseded by this comment** — item 2 is disproved and item 3 is answered in the negative. ## `max_predicate_cache_size` does not help Two layers had to be fixed before this could be measured at all: 1. `spark.comet.datafusion.*` is gated behind `spark.comet.exec.respectDataFusionConfigs`, which defaults to `false` (`CometConf.scala:839`). Without it, the prefix is silently dropped. Confirmed by setting only `spark.comet.datafusion.execution.parquet.pushdown_filters=true`: the scan still emitted all 600,037,902 rows. 2. Even with that flag on, the value still never reaches the reader. `get_options()` builds `TableParquetOptions::new()` from scratch (`parquet_exec.rs:218`), while `ParquetSource` reads the setting from `table_parquet_options.global` (`datafusion-datasource-parquet-54.0.0/src/source.rs:465`). Filed separately as #4990. With a local patch seeding `table_parquet_options.global` from the session config, and `reorder_filters` held at `false`, TPC-H SF100 Q6, median of the last 5 of 10 iterations: | `max_predicate_cache_size` | median | vs pushdown off (0.853 s) | | --- | --- | --- | | 0 (disabled) | 2.360 s | +176.7% | | 100 MB (default) | 2.290 s | +168.5% | | 1 GB | 2.305 s | +170.2% | Verified genuinely applied this time: `cache=0` produced no predicate-cache metrics at all, while the others reported 48.8B cached reads. So the cache is worth roughly 3%, is already saturated at its 100 MB default, and raising it changes nothing. This is consistent with the root cause: the cache avoids *re-decoding* filter columns, whereas Q6's problem is that those columns must be fully decoded in the first place. Exposing the knob is still reasonable for tunability, but it is not a fix for this regression. ## `reorder_filters` has no measurable effect Same setup, default cache, Q6: | config | median | | --- | --- | | `reorder_filters=true` | 2.302 s | | `reorder_filters=false` | 2.303 s | Item 2 above proposed decoupling `reorder_filters` on the theory that it might be responsible for part of the regression. On Q6 it makes no difference at all. Decoupling it may still be worth doing for clarity, but it should not be presented as a performance fix. ## Caveat on the absolute numbers in this comment The patch used to make `max_predicate_cache_size` reachable itself cost ~51% on the pushdown-on path (1.52 s -> 2.30 s on Q6). Reverting and re-measuring returned it to 1.517 s, so the attribution is solid, but the cause is unexplained: `pushdown_filters` was already effectively enabled via `table_pushdown_enabled || config_pushdown_enabled` (`source.rs:740`), and `reorder_filters` measures as a no-op. That is covered in #4990. Every figure in this comment therefore carries that ~51% penalty and is only meaningful *relative to the others in its own table*. The headline regression for Q6 from an unpatched build remains **0.853 s -> 1.517 s (+78%)**, as in the description. ## Net effect on the proposed work items - Item 1 (drop the redundant `CometFilter` when all filters push down) is unaffected and remains the most valuable item. - Item 2 (decouple `reorder_filters`) is disproved as a performance fix. - Item 3 (expose `max_predicate_cache_size`) is answered: it does not help, and it is currently unreachable regardless (#4990). - Item 4 (make this a planner decision on projection width and selectivity) is unaffected and, given the above, is now clearly the only route to enabling this by default. -- 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]
