NoahKusaba commented on code in PR #14: URL: https://github.com/apache/datafusion-iceberg/pull/14#discussion_r4077204381
########## crates/datafusion/src/physical_plan/project.rs: ########## Review Comment: Not intentional — fixed in f726703. Worse than it looked, in fact: `try_new` allocates a fresh calculator `Arc` on every call, so the old impl made a `PartitionExpr` equal only to direct clones of itself. Pointer equality was fine while `new` was private and each plan held one instance; making the type reconstructible is what breaks that. Semantics now: two `PartitionExpr` are equal when they would compute the same partition values, i.e. their partition spec and table schema are equal. The calculator is derived from those two, so it no longer participates. `Hash` uses `spec_id` + `schema_id`, since neither type implements `Hash` — equal expressions agree on both, and collisions fall through to `eq`. Tests now assert equality and hash equality across a deep-copy rebuild, plus a case sharing a `spec_id` but partitioning on different columns so `eq` can't narrow to the ids. Also documented the three newly-public methods. CI hasn't run on any of my pushes — I think it needs maintainer approval for a first-time contributor here. `fmt`, `clippy -D warnings` and `cargo test --workspace --locked` are clean locally. -- 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]
