Uh oh!
There was an error while loading. Please reload this page.
Core: Don't override equals() and hashCode() in V4 DeletionVector - #17447
Conversation
405ddb5 to
544114fComparegaborkaszab
commented
Jul 31, 2026
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.
kevinjqliu
commented
Aug 1, 2026
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
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.
nssalian
commented
Aug 3, 2026
@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. |
gaborkaszab
commented
Aug 3, 2026
Thanks for looking, @anoopj , @kevinjqliu , @uros-b , @nssalian ! |
kevinjqliu
commented
Aug 3, 2026
thanks for the context! ❤️ |
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
Uh oh!
There was an error while loading. Please reload this page.
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.