rdblue commented on code in PR #16769:
URL: https://github.com/apache/iceberg/pull/16769#discussion_r3463448205
##########
core/src/main/java/org/apache/iceberg/DeletionVectorStruct.java:
##########
@@ -128,6 +129,26 @@ static Builder builder() {
return new Builder();
}
+ @Override
+ public boolean equals(Object other) {
Review Comment:
I think that we should remove this. There are multiple ways of considering
something "equal" and we try to avoid `equals` methods when it is unclear which
one should be used. That's the case here, where it would be reasonable to
assume that two `DeletionVector` instances are the same if they represent the
same DV (`location` and `offset` are equal) but also reasonable to assume two
instances are equal if they have the same values (mostly implemented here).
In addition to the problem of what this means, there's another problem that
this definition only works for other `DeletionVectorStruct` instances. That
means the definition of equality is more murky: two `DeletionVector` instances
can be equal by either definition above but still may not be equal here. The
purpose of using interfaces for the public API is to make it so that
implementations are interchangeable and this breaks that principle.
I think this was introduced to be able to detect changes applied to the
builder, but we should find a better way to solve that problem rather than
introducing this.
--
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]
