Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[#2286] Distribution.java correct equals method. #4507

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

lokendra005
Copy link

What changes were proposed in this pull request?

This pull request addresses the issue with the equals method in Distribution.java. The equals method is updated to correctly override Object.equals(Object), ensuring it adheres to Java's best practices.

Why are the changes needed?

This pull request addresses the issue with the equals method in Distribution.java. The equals method is updated to correctly override Object.equals(Object), ensuring it adheres to Java's best practices.

Fix: #2286

Does this PR introduce any user-facing change?

NO

How was this patch tested?

This patch was tested using unit tests that verify the behavior of the equals.

@lokendra005
Copy link
Author

@diqiu50 I have raised this new pull request this doesn't change the original behavior, kindly review.

@diqiu50
Copy link
Contributor

diqiu50 commented Aug 14, 2024

@diqiu50 Can you compile the code successfully,

@diqiu50
Copy link
Contributor

diqiu50 commented Aug 14, 2024

This equals method is primarily used to solve compare different instances of DistributionDTO and DistributionImpl. The equals are the best name. If everyone feels that this name is ambiguous, we can also change it.

You need to add some testers for it

@justinmclean
Copy link
Member

It may be that this needs to be fixed?

/home/runner/work/gravitino/gravitino/api/src/main/java/org/apache/gravitino/rel/expressions/distributions/Distribution.java:60: warning: [cast] redundant cast to Distribution
    Distribution that = (Distribution) obj;

@lokendra005
Copy link
Author

lokendra005 commented Aug 14, 2024

@justinmclean @diqiu50 can I make a separate class for distribution??

public class DistributionImpl implements Distribution {

@Override
public boolean equals(Object obj) {
    if (this == obj) {
        return true;
    }
    if (obj == null || getClass() != obj.getClass()) {
        return false;
    }

    Distribution that = (Distribution) obj;
    return number() == that.number()
        && strategy().equals(that.strategy())
        && Arrays.equals(expressions(), that.expressions());
}

}

@diqiu50
Copy link
Contributor

diqiu50 commented Aug 19, 2024

@lokendra005 Can you update you code for review. You need add some tester for this changed. I expect you have some testers that compare difference object for Distribution

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Improvement] In Distribution.java correct equals method
3 participants