namanjain24-sudo commented on PR #25091: URL: https://github.com/apache/datafusion/pull/25091#issuecomment-5761011615
Thanks @kosiew, good catch. Pushed 650a5de1b: - Dropped the metadata-equality conditions: a field is read from the right whenever the left leaves it nullable and the right requires it, and the schema-metadata fallback to `LogicalPlanBuilder::intersect` is gone, so the narrowing path is always taken. - A column read from the right is aliased with the left field's metadata. An inner join's schema (logical and physical) already lets the left side's schema metadata win, so on conflicting keys the left's metadata wins. - `intersect_nullability` now runs each plan a second time over tables with differing schema and field metadata. It checks that the logical, physical and collected-batch schemas agree, and that the primary's metadata wins where the tables disagree. It fails without the change (`a?` stays nullable). Two things to flag: 1. Alias metadata is merged over the right field's metadata, not replacing it, so keys present only on the right (field or schema level) still appear in the result. I first tried stating the exact left schema with an explicit-schema `Projection`, but the optimizer re-derives it and the physical plan then disagreed with the logical one. Happy to go another way if you want exact-left semantics. 2. While writing the test I saw that a `Union` of inputs with differing field metadata reports different metadata in its logical schema (last input's) than in its physical schema (first input's), which tripped the aggregate schema check. It's independent of this change, so the test gives the secondary tables the same metadata. I haven't root-caused it; I can file a separate issue. Locally I ran `cargo test -p datafusion-substrait`, `cargo fmt`, and clippy with `--all-features -D warnings` on that crate (with `-A clippy::unused_async_trait_impl`, which `datafusion-datasource-parquet` trips on my toolchain; not touched here). I didn't run the full workspace suite and am relying on CI for it. I also updated the PR description, which still said a field is only read from the right when its metadata matches. -- 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]
