Core: Don't override equals() and hashCode() in V4 DeletionVector - #17447
Conversation
405ddb5 to
544114f
Compare
anoopj
left a comment
anoopj
left a comment
There was a problem hiding this comment.
The change looks good to me. We don't rely on this currently so let's remove it.
|
im curious, what is the reasoning to remove these? It seems useful. Would it be a surprise later on when comparing I see it was added originally in 7c13104#diff-44ec3b66acad90f4f873b3786f55feb62bb03287626b41bd4622c11d5ca9963fR132-R150 |
| } | ||
|
|
||
| @Override | ||
| public boolean equals(Object other) { |
There was a problem hiding this comment.
No comment documents the intended equality contract after this change. Equality is now identity-based, and the safety of that choice rests on the invariant that no caller puts DeletionVectorStruct in a Set/Map key or calls .equals() for value comparison. As key_metadata (#17438) and further V4 fields land, a future reader writing dedup/caching code could reasonably assume value equality is in place. A single class-level sentence; e.g. "Equality and hash code are identity-based; DeletionVectorStruct is a projection-backed view, not a value type" would protect this invariant. This is the same documentation gap present in none of the sibling structs (TrackedFileStruct etc.), so adding it here sets a useful precedent for the whole V4 family.
uros-b
left a comment
uros-b
left a comment
There was a problem hiding this comment.
@gaborkaszab Please note that the PR body is currently empty: no description, no linked issue, no rationale. Iceberg convention expects a description explaining why the override is being removed.
|
@gaborkaszab the Rationale for this change isn't clear. Please add more details and context since reviewers here might not be aware. Happy to take a look once the reasoning is fleshed out. |
|
Thanks for looking, @anoopj , @kevinjqliu , @uros-b , @nssalian ! |
|
thanks for the context! ❤️ |
amogh-jahagirdar
left a comment
amogh-jahagirdar
left a comment
There was a problem hiding this comment.
Yeah thanks for cleaning this up @gaborkaszab! I will go ahead and merge
6 participants
The convention for V4 metadata structs like
DeletionVectorStructis that we don't overrideequals()andhashCodeonly if it's inevitable. See this comment on a previous PR. These overrides were meant to be removed/not added in that PR, however, somehow went under the radar and made it in.This PR removes unintentionally adding the overrides.