Adding Proto-based equality and comparison #1157
Conversation
This will be used (and tested) in the follow-up CL that adds the FieldValue tpes.
Codecov Report
Continue to review full report at Codecov.
|
| import java.util.Map; | ||
| import java.util.TreeMap; | ||
|
|
||
| public class ProtoValueUtil { |
There was a problem hiding this comment.
Same comment about naming: how about ProtoValues?
|
|
||
| switch (value.getValueTypeCase()) { | ||
| case NULL_VALUE: | ||
| return FieldValue.TYPE_ORDER_NULL; |
There was a problem hiding this comment.
Putting this class in the util package creates a cyclic reference between packages that isn't going to work in C++. Since we're using these utilities in the service of building models, might as well just put this in the model package.
There was a problem hiding this comment.
Done. I believe that means that this class can then be package-private. I did not yet make it so since my next PR temporarily adds a new package for the new ObjectValue to avoid breaking all existing code. Added a TODO to change the visibility.
| switch (leftType) { | ||
| case FieldValue.TYPE_ORDER_ARRAY: | ||
| return arrayEquals(left, right); | ||
| case FieldValue.TYPE_ORDER_NUMBER: |
There was a problem hiding this comment.
Nit: consider ordering the switch cases consistently so that it's easier to audit/reason about differences (in this case, that would mean number, array, object).
There was a problem hiding this comment.
Done (here and everywhere). I am following the type order constants.
| } | ||
| } | ||
|
|
||
| private static boolean objectEquals(Value left, Value right) { |
There was a problem hiding this comment.
Nit: consider ordering these methods in the file in the order you're using them in equals, above.
| } | ||
| } | ||
|
|
||
| private static int compareMaps(Value left, Value right) { |
There was a problem hiding this comment.
Nit: consider ordering these to match their usage above.
| double thisDouble = left.getDoubleValue(); | ||
| if (right.getValueTypeCase() == Value.ValueTypeCase.DOUBLE_VALUE) { | ||
| return Util.compareDoubles(thisDouble, right.getDoubleValue()); | ||
| } else { |
There was a problem hiding this comment.
You could express this with less code if you handled all the unexpected cases in one place, below:
if (left is double) {
if (right is double) {
return ...;
} else if (right is int) {
return ...;
}
} else if (left is int) {
if (right is double) {
return ...;
} else if (right is int) {
return ...;
}
}
hardAssert("Unexpected types %s and %s, left, right);
There was a problem hiding this comment.
Great suggestion. Updated.
| // Byte values are equal, continue with comparison | ||
| } | ||
| return Util.compareIntegers(bytes.size(), other.bytes.size()); | ||
| return Util.compareByteString(bytes, other.bytes); |
There was a problem hiding this comment.
Other compare methods are plural (e.g. compareIntegers). compareByteStrings?
| int minLength = | ||
| Math.min(left.getArrayValue().getValuesCount(), right.getArrayValue().getValuesCount()); | ||
| private static int compareTimestamps(Value left, Value right) { | ||
| if (left.getTimestampValue().getSeconds() == right.getTimestampValue().getSeconds()) { |
There was a problem hiding this comment.
Elsewhere we compare and then if non-zero do the second one. Consider rephrasing as:
int cmp = Util.compareIntegers(left.getTimestampValue().getSeconds(), right.getTimestampValue().getSeconds());
if (cmp != 0) return cmp;
return Util.compareIntegers(left.getTimestampValue().getNanos(), right.getTimestampValue().getNanos());This has two benefits:
- it makes the hierarchy of comparison linear; and
- it avoids using subtraction for comparison, which is specious in the general case due to the possibility of overflow. I think it's the case that timestamp values are defined in such a way that subtraction can't possibly overflow, but there's no reason not to reuse the comparator we already have, in which case we don't have to think about it.
There was a problem hiding this comment.
This code is actually based on the original code. I do think the comparisons in the other functions are easier to follow, so I updated it.
As part of this, I changed the method to take TimestampValues (along with the rest). This avoids a bunch of extra linebreaks since it removes the calls to getTimestampValue() from here.
schmidt-sebastian
force-pushed
the
mrschmidt/comparisons
branch
from
66d722f to
6e4ddde
Compare
|
@schmidt-sebastian: The following test failed, say
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
This will be used (and tested) in the follow-up CL that adds the FieldValue tpes.
I am also removing one of the the two "compareInt" functions.
For reference: #1156