namanjain24-sudo commented on PR #25091: URL: https://github.com/apache/datafusion/pull/25091#issuecomment-5734084511
Thanks @kosiew, you were right. I added the assertion you suggested and it failed on the previous commit: the logical schema had `a` as required, but `create_physical_plan().schema()` still had it nullable, because `ProjectionExec` takes only metadata from the supplied schema. Instead of a rebinding exec or a change to `ProjectionExec`, the latest commit removes the need to override the schema at all: - A new execution plan or schema adapter would need its own logical node, and `DefaultPhysicalPlanner` can't plan that unless an `ExtensionPlanner` is registered. A consumed Substrait plan has to run in any `SessionContext`. - Letting `ProjectionExec` narrow nullability would change every projection in core. The narrowing would also have to survive the physical optimizer's rebuild and removal paths, like the metadata override from #23981 does. That felt like too much for a Substrait consumer fix. What it does now: when the right input requires a field that the left input leaves nullable, the intersection is built as an inner join (nulls equal nulls) against the distinct right rows, and that field is read from the right side. Matched rows hold equal values, so the result rows don't change. The field is non-nullable because its source column is, and both the logical and the physical planner derive that from the input schema, so the two stay consistent without any override. Joining against distinct right rows keeps each left row at most once, the same as the semi join. A field is only taken from the right when its type and metadata match the left's (and the schema metadata matches too), so the result's attributes stay the left input's. When nothing needs narrowing, it still uses `LogicalPlanBuilder::intersect` as before. Tests: - The tables now have rows (including nulls). For each of the three plans, the test checks that the logical schema, `create_physical_plan().schema()` and the schema of every collected batch are equal, and checks the result rows. - Added `NULLABILITY_UNSPECIFIED` on a secondary input (columns `e` and `f`). `e` would come out required for the multiset intersections, and `f` required for `INTERSECTION_PRIMARY`, if unspecified were read as required. -- 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]
