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
Conversation
Double.NaN and Float.NaN are considered equal when wrapped with the Float64 Value implementation class.
@@ -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; |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 Mutation
s 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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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))) { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
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. |
No description provided.