andygrove commented on PR #4587: URL: https://github.com/apache/datafusion-comet/pull/4587#issuecomment-5441787839
> **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. Mapping Spark's `ExistenceJoin` onto DataFusion's `LeftMark` is a neat idea and the change is refreshingly small. Gating it behind a disabled-by-default config while it settles is the right call, and the `producedAttributes` overrides are the detail that would have been easy to miss. Before this can be reviewed properly, though, a few things. **The description is empty** Every section of the template is still a placeholder: no rationale, no summary of changes, no testing notes. For a change that introduces a new join type mapping across three join operators, the reviewer needs to know what you verified. In particular I would want the description to answer the semantic question below, because a reader cannot get that from the diff. **Is `LeftMark` semantically identical to `ExistenceJoin`?** Spark's `ExistenceJoin(exists)` emits every left row plus a boolean that is true when at least one right row matches the equi keys *and* the join condition. Three things I would want confirmed against DataFusion's `LeftMark`: - Does `LeftMark` evaluate the non-equi `condition` before setting the mark? Spark's `ExistenceJoin` is most often planned for `EXISTS ... OR ...` shapes, which is exactly where a non-trivial condition appears. If DataFusion marks on the key match alone and filters afterwards, the results diverge. - Can the mark column ever be NULL? Spark's `exists` attribute is `nullable = false`. If `LeftMark` can produce a null, downstream expressions will behave differently. - Where does the mark land in the output schema, and does that match `left.output :+ exists`? A short section in the description walking through those, ideally with a DataFusion source link, would make this reviewable. **`BroadcastNestedLoopJoinExec` is not covered** `ExistenceJoin` is added to `CometHashJoin` and `CometSortMergeJoinExec`, but Spark frequently plans an existence join as a broadcast nested loop join when there are no equi-join keys, which is the common shape for `WHERE EXISTS (...) OR x > 1`. Is that deliberately out of scope? If so, saying so would help; if not, those queries will silently keep falling back and the feature will look like it does not work. **Benchmark results** `CometExistenceJoinBenchmark` is added but the description has no numbers. Since the motivation for a native existence join is presumably speed, what does it buy over the fallback? **One CI note** There is a failing check on this PR. Worth looking at before the next round of review. -- 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]
