NoahKusaba commented on code in PR #14:
URL: https://github.com/apache/datafusion-iceberg/pull/14#discussion_r4085433998
##########
crates/datafusion/src/physical_plan/project.rs:
##########
@@ -181,9 +223,11 @@ impl std::fmt::Display for PartitionExpr {
impl std::hash::Hash for PartitionExpr {
fn hash<H: std::hash::Hasher>(&self, state: &mut H) {
- // Two PartitionExpr are equal if they share the same calculator and
partition_spec Arcs
- Arc::as_ptr(&self.calculator).hash(state);
- Arc::as_ptr(&self.partition_spec).hash(state);
Review Comment:
Fair question — I went looking, and I don't think there was a reason.
It came in with `b3b5afed`, the commit that first added `PartitionExpr`
(apache/iceberg-rust#1602). The comment there was just `// Manual PartialEq/Eq
implementations for pointer-based equality`, with no rationale, and equality
wasn't discussed anywhere in that PR's review.
My read is that it was mechanical rather than deliberate:
`PartitionValueCalculator` derives only `Debug`, so `PartialEq` can't be
derived on a struct holding one, and `ptr_eq` is the path of least resistance.
Worth noting `PartitionSpec` does implement `PartialEq`, so the spec could have
been compared by value even then — it looks like it just got swept along with
the calculator.
So I don't think we're overriding a deliberate choice; dropping the
calculator from the comparison removes the constraint that forced pointers in
the first place. It's derived from the spec and schema, so it carries no
information those two don't already have.
Happy to be wrong if anyone knows otherwise — cc @flaneur2020 / @Fokko in
case there's history I can't see from the commit.
--
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]
