Skip to content

HIVE-28875 Numbers should be compared with equals(), not with == #5743

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

dk2k
Copy link

@dk2k dk2k commented Apr 2, 2025

No description provided.

Copy link

@aturoczy aturoczy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I see, you have made several PR for equals change. Please wait for @ayushtkn and @deniskuzZ recommendation about the style. If it meets the community standard, I think this PR's could be 1 "big" does not need to separate to 10+ small.

@SourabhBadhya
Copy link
Contributor

equals() is used to check the object content, == does a comparision on the object reference. Using equals() is the right programming paradigm.

I did a small test for confirming the same for Double datatype - There is a difference -

Double do1 = 9.0;
Double do2 = 9.00;
System.out.println(do1 == do2); // Returns false
System.out.println(do1.equals(do2)); // Returns true

I agree with @aturoczy comment. Instead of creating small PRs for handling each of these minor details its better to handle it in a single PR. Its always better to group similar ones and post aggregated PRs on them.

@dk2k
Copy link
Author

dk2k commented Apr 20, 2025

@SourabhBadhya @aturoczy I didn't mean to overwhelm the repo with PRs. But still I prefer to have several atomic PRs because it's easier to complete each PR. As for equals(): it has some specifics for Strings and numbers. If both strings are interned (which I didn't check), it's ok to compare their values using ==

@SourabhBadhya
Copy link
Contributor

@dk2k Please rebase your PR for the tests to pass.

@dk2k
Copy link
Author

dk2k commented Apr 21, 2025

@SourabhBadhya I can merge master branch. Is it what you need?

@SourabhBadhya
Copy link
Contributor

@dk2k Yes you can do that as well.

@dk2k
Copy link
Author

dk2k commented Apr 23, 2025

@SourabhBadhya I merged

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

Successfully merging this pull request may close these issues.

4 participants