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

@thiagotnunes thiagotnunes commented Nov 15, 2021

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 as a code owner Nov 15, 2021
@google-cla google-cla bot added the cla: yes label Nov 15, 2021
@product-auto-label product-auto-label bot added the api: spanner 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
Contributor

@olavloite olavloite Nov 15, 2021

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

@thiagotnunes thiagotnunes Nov 15, 2021

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
Contributor

@olavloite olavloite Nov 15, 2021

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

@libingye816 libingye816 Nov 15, 2021

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
Contributor

@olavloite olavloite Nov 15, 2021

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

@thiagotnunes thiagotnunes Nov 16, 2021

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

@libingye816 libingye816 Nov 16, 2021

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

@thiagotnunes thiagotnunes Nov 16, 2021

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

@libingye816 libingye816 Nov 16, 2021

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

@thiagotnunes thiagotnunes Nov 16, 2021

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
Contributor

@olavloite olavloite Nov 16, 2021

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

@thiagotnunes thiagotnunes Nov 16, 2021

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 label Nov 16, 2021
@gcf-merge-on-green
Copy link
Contributor

@gcf-merge-on-green gcf-merge-on-green bot commented Nov 16, 2021

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 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
20 checks passed
@thiagotnunes thiagotnunes deleted the value-float64-nans branch Nov 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: spanner cla: yes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants