andygrove commented on PR #4810: URL: https://github.com/apache/datafusion-comet/pull/4810#issuecomment-5876338212
This is a light fully automated review since there are so many PRs open. 1. `dynamic_filter_rows_pruned` and `dynamic_filter_eval_time` never reach the Spark UI. The wrapper registers them at `native/core/src/execution/operators/dynamic_filter.rs:134` and `:140`, and the planner merges them into the join node. But `CometHashJoinExec` and `CometBroadcastHashJoinExec` take their metric map from `CometMetricNode.joinMetrics` (`CometMetricNode.scala:306`), which declares neither name. `CometMetricNode.set` drops unknown names with only a debug log. So someone who turns the flag on can't tell whether it pruned anything. Could both be added to `joinMetrics`? That would also let the new `CometJoinSuite` test assert `dynamic_filter_rows_pruned > 0` for the broadcast inner join (`df1`). Today that test passes whether or not the filter gets attached. 2. The config doc at `CometConf.scala:379` says the filter applies to left anti joins, but I don't think a Spark anti join can ever get it. Spark never builds the left side of an anti join (neither `canBuildBroadcastLeft` nor `canBuildShuffledHashJoinLeft` admits `LeftAnti`, in 3.4 through 4.1). So the non-null-aware case takes the `swap_inputs` branch at `planner.rs:2006`, becomes `RightAnti`, and `attach_join_dynamic_filter` skips it at `dynamic_filter.rs:288` because `on_lr_is_preserved().1` is false. The null-aware case is excluded explicitly. Left outer is narrower than the doc suggests too. A broadcast-right `LEFT JOIN` becomes `Right` after the swap and is skipped. Outer joins only qualify when the build side is the preserved side, which Spark only plans as a shuffled hash join on 3.5+, and that also covers right outer joins. Would it make sense to describe eligibility in Spark terms? The `df6` comment at `CometJoinSuite.scala:280` makes the same assumption. -- 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]
