fix(spanner): compare null elements in primitive-array Value.equals#13826
Open
fornwall wants to merge 1 commit into
Open
fix(spanner): compare null elements in primitive-array Value.equals#13826fornwall wants to merge 1 commit into
fornwall wants to merge 1 commit into
Conversation
The five primitive-array Value implementations (BoolArrayImpl, Int64ArrayImpl, Float32ArrayImpl, Float64ArrayImpl, PgOidArrayImpl) compared only the backing primitive array in valueEquals, ignoring the BitSet that tracks which elements are null. Null elements are stored as the primitive default in the backing array, so for example int64Array(asList(null, 1L)) compared equal to int64Array(asList(0L, 1L)) even though the two serialize to different protos and (since valueHash does include the null mask) have different hash codes. This breaks the equals/hashCode contract, use of Value in hash-based collections, and Mutation.equals-based mutation diffing. Move the null-mask comparison into a shared valueEquals override on PrimitiveArrayImpl, mirroring the existing valueHash/arrayHash split, and turn the subclass overrides into arrayEquals. Signed-off-by: Fredrik Fornwall <fredrik@fornwall.net>
Contributor
There was a problem hiding this comment.
Code Review
This pull request fixes an issue in Value.java where equality checks for primitive arrays did not correctly account for null elements. Since null elements are stored as primitive defaults in the backing array, the valueEquals method in PrimitiveArrayImpl was updated to compare both the nulls bitmask and the backing arrays via a new abstract arrayEquals method. Corresponding subclass implementations were updated, and comprehensive equality tests were added in ValueTest.java to verify correct behavior. There are no review comments, so I have no feedback to provide.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The five primitive-array
Valueimplementations (BoolArrayImpl,Int64ArrayImpl,Float32ArrayImpl,Float64ArrayImpl,PgOidArrayImpl) implementedvalueEqualsasArrays.equals(values, that.values)on the backing primitive array only.Null elements are tracked in a separate
BitSet nullsand stored as the primitive default (0,0.0,false) in the backing array, so the null mask was never part of equality:Consequences:
equalsreturned the wrong answer — it reported semantically distinct values as equal:[null, 1]and[0, 1]serialize to different protos viavalueToProto()(null elements becomeNULL, not the primitive default) and are visibly different throughgetInt64Array().equals/hashCodecontract violation —PrimitiveArrayImpl.valueHash()does include the null mask (31 * Objects.hashCode(nulls) + arrayHash()), so values that compared equal hashed differently, breakingHashMap/HashSetusage ofValue,Struct, andMutation.Mutation.equalsto detect modifications before committing. An update that changes[0, 1]to[NULL, 1](or vice versa) compared as "no change" and can be silently dropped.Fix that by moving the null-mask comparison into a single shared
valueEqualsoverride onPrimitiveArrayImpl, mirroring the existingvalueHash()/arrayHash()split, and turn the per-subclassvalueEqualsoverrides intoarrayEquals.