buraksenn commented on PR #24045: URL: https://github.com/apache/datafusion/pull/24045#issuecomment-5156938617
> The fix looks correct to me. The asymmetry is the crux: for a semi join every emitted row has an equal key on the non-output side, so pushing an output-key filter there only removes rows that wouldn't have been emitted; for an anti join an emitted row means _no_ match on that side, so filtering it changes which left rows fail to match. Restricting anti joins to the output side (`LeftAnti → (true, false)`, `RightAnti → (false, true)`) while keeping semi at `(true, true)` is the right call, and it's the physical-plan analog of the logical `push_down_filter` restriction. > > One thing I'd like to see before this lands: **an end-to-end test that asserts the query results, not just the plan.** Right now both the unit test (`assert_parent_filter_remains`) and the updated SLT are plan-level — they check that the `FilterExec` stays / that the dynamic filter no longer appears on the right scan. But the bug in #24002 is a _wrong-results_ bug (a `LeftAnti` emitting every build-side row instead of zero), and nothing here actually runs a query and checks the row count. > > Could you add an SLT that executes the failing shape and asserts the output — e.g. the constant-false-over-`LeftAnti` case from the issue returning 0 rows, and/or an anti join whose right side would be filtered, asserting the correct rows? That would pin the actual correctness regression (a future refactor could keep the plan shape happy while reintroducing the wrong result). Happy to help put one together if useful. > > The code change itself is 👍 — just want the result-level coverage before approving. Thanks for the review @viirya. I've just added an SLT test that will fail without this fix thanks for heads up about it. -- 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]
