andygrove commented on PR #4810: URL: https://github.com/apache/datafusion-comet/pull/4810#issuecomment-5441823951
> **Note on this review:** this was generated by an LLM (Claude Code) at my request while I worked through a review backlog. I have not verified the individual findings myself. Please treat everything below as suggestions to evaluate rather than as authoritative review feedback, and push back on anything that is wrong or already handled. This is a substantial, well-argued piece of work. Wiring DataFusion 54's dynamic join filtering into Comet's own planner, since Comet does not run DataFusion's physical optimizer, is exactly the right observation, and doing it scan-agnostically so every probe-side source inherits it is the right level to intervene at. The eligibility gate mirroring `JoinType::on_lr_is_preserved().1` rather than a hand-written list, and the note about null-aware anti joins, both suggest the semantics were checked properly. Some things to resolve. **CI is red** `PR Build (Linux) / ubuntu-latest/rust-test` and `PR Benchmark Check / Benchmark Compile & Lint Check` are both failing. Worth fixing before the next round, since a reviewer cannot tell what else the failures are masking. **No end-to-end performance results** The motivating claim is that build-side runtime filtering is "one of the largest TPC-DS levers in comparable engines", and a 167-line benchmark is included, but the description has no numbers. This is a big feature to take on and the case for it rests entirely on the speedup. Could you run the benchmark and put the results in the description, and ideally a TPC-DS run with the config on and off? Both the wins and the queries where it costs more than it saves are useful. **The selectivity guard's constants** The guard disables evaluation on a partition stream when the filter keeps more than 95% of rows after 64K rows. Two questions: - Where do 95% and 64K come from? Some justification would help, even if it is "these are what Velox uses" or "measured on the benchmark above". - The guard is per-partition-stream and permanent once tripped. Is there a case where a filter is unselective early and highly selective later, for example when the probe side is sorted on the join key? Permanently disabling on the first 64K rows would then give up most of the benefit. A guard that periodically re-samples would be more robust, though also more complex. **The `ProjectionExec`-over-join shape** `attach_join_dynamic_filter` handles the `swap_inputs` shape by looking through a `ProjectionExec`. That is a structural pattern match on a plan shape DataFusion produces today. If a future DataFusion version changes what `swap_inputs` emits, the helper silently stops attaching the filter and the feature quietly turns off with no signal. Would it be worth logging at debug level when an eligible-looking join is skipped because the shape did not match, so this is diagnosable? **Default-off is right, but for how long?** Marking it experimental and default-off is the right call for a first landing. It would help to say in the description what would have to be true to flip the default: benchmark coverage, a period on main, a specific set of TPC-DS results. -- 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]
