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

feat: NaNs in Mutations are equal and have the same hashcode #1554

Merged
merged 6 commits into from Nov 16, 2021

Conversation

thiagotnunes
Copy link
Contributor

No description provided.

Double.NaN and Float.NaN are considered equal when wrapped with the
Float64 Value implementation class.
@thiagotnunes thiagotnunes requested a review from a team as a code owner November 15, 2021 06:05
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Nov 15, 2021
@product-auto-label product-auto-label bot added the api: spanner Issues related to the googleapis/java-spanner API. label Nov 15, 2021
@@ -1028,7 +1028,8 @@ void valueToString(StringBuilder b) {

@Override
boolean valueEquals(Value v) {
return ((Float64Impl) v).value == value;
final Float64Impl float64Value = (Float64Impl) v;
return Double.isNaN(value) && Double.isNaN(float64Value.value) || float64Value.value == value;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we want to deviate from the IEEE 754 standard here? According to that (and also according to the Cloud Spanner backend) NaN == NaN should return false.

The following SQL statement for example returns false:

select (IEEE_DIVIDE(0.0, 0.0) = IEEE_DIVIDE(0.0, 0.0)) as b

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is a good point and I considered this.

The issue that we have is that on import / export in Apache Beam (see here) we compare mutations before committing. Since we allow for PGNumerics that allow for such a value, import / export is failing and that is why we are loosening the restriction here.

If you feel strong that we should not make changes here, we could try and explore a change in Apache Beam itself, but I guess we would just be re-implementing equality checks over there.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is maybe a slightly hypothetical problem, but I don't think we should implement this in Value. However, it might make sense to implement it in Mutation (if doable). That is:

If someone executes the following query:

SELECT FloatValue1, FloatValue2
FROM SomeTable
WHERE FloatValue1 != FloatValue2

Then resultSet.getValue("FloatValue1).equals(resultSet.getValue("FloatValue2")) should return false for all rows.

On the other hand, for Mutations the difference between two NaN values is void, as they will both set the value of a row to the same. So:

      Mutation mutation1 = Mutation.newInsertBuilder("SomeTable")
          .set("FloatValue").to(Value.float64(Double.NaN))
          .build();
      Mutation mutation2 = Mutation.newInsertBuilder("SomeTable")
          .set("FloatValue").to(Value.float64(Double.NaN))
          .build();
      // The following could (should?) be true as both mutations will have the same effect.
      assertTrue(mutation1.equals(mutation2));

Would that solve the problem in Beam?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, this would solve the problem for us. In addition, is it possible to see Double.NaN in PgNumeric Value?

Copy link
Collaborator

Choose a reason for hiding this comment

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

In addition, is it possible to see Double.NaN in PgNumeric Value?

Yes, that is possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed the implementation to check for NaNs only in mutations

Copy link
Member

Choose a reason for hiding this comment

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

In addition, is it possible to see Double.NaN in PgNumeric Value?

Yes, that is possible.

Hi @thiagotnunes , is this case considered? I see we only try to cast the type to float64 but I am not an expert on client libraries. Ignore me if I am wrong.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey Michael, since this fix is on the public repository we don't have pg numeric here yet. I will make the necessary changes in the other repository as well.

Copy link
Member

Choose a reason for hiding this comment

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

oh, yes. Sorry, you are absolutely right.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No worries!

for (int i = 0; i < values.size(); i++) {
final Value value = values.get(i);
final Value otherValue = otherValues.get(i);
if (!value.equals(otherValue) && (!isNaN(value) || !isNaN(otherValue))) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: maybe for future reference we should add a short comment on why we are doing this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, added a comment here

@thiagotnunes thiagotnunes added the automerge Merge the pull request once unit tests and other checks pass. label Nov 16, 2021
@gcf-merge-on-green
Copy link
Contributor

Merge-on-green attempted to merge your PR for 6 hours, but it was not mergeable because either one of your required status checks failed, one of your required reviews was not approved, or there is a do not merge label. Learn more about your required status checks here: https://help.github.com/en/github/administering-a-repository/enabling-required-status-checks. You can remove and reapply the label to re-run the bot.

@gcf-merge-on-green gcf-merge-on-green bot removed the automerge Merge the pull request once unit tests and other checks pass. label Nov 16, 2021
@thiagotnunes thiagotnunes changed the title feat: NaNs wrapped in Float64 Values are equal and have the same hashcode feat: NaNs in Mutations are equal and have the same hashcode Nov 16, 2021
@thiagotnunes thiagotnunes merged commit 91a18fc into main Nov 16, 2021
@thiagotnunes thiagotnunes deleted the value-float64-nans branch November 16, 2021 21:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: spanner Issues related to the googleapis/java-spanner API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants