Skip to content

fix(spanner): compare null elements in primitive-array Value.equals#13826

Open
fornwall wants to merge 1 commit into
googleapis:mainfrom
fornwall:fix/p0-1-primitive-array-value-equals-nulls
Open

fix(spanner): compare null elements in primitive-array Value.equals#13826
fornwall wants to merge 1 commit into
googleapis:mainfrom
fornwall:fix/p0-1-primitive-array-value-equals-nulls

Conversation

@fornwall

Copy link
Copy Markdown

The five primitive-array Value implementations (BoolArrayImpl, Int64ArrayImpl, Float32ArrayImpl, Float64ArrayImpl, PgOidArrayImpl) implemented valueEquals as Arrays.equals(values, that.values) on the backing primitive array only.

Null elements are tracked in a separate BitSet nulls and stored as the primitive default (0, 0.0, false) in the backing array, so the null mask was never part of equality:

 // Returned true:
Value.int64Array(Arrays.asList(null, 1L)).equals(Value.int64Array(Arrays.asList(0L, 1L)))

Consequences:

  • equals returned the wrong answer — it reported semantically distinct values as equal: [null, 1] and [0, 1] serialize to different protos via valueToProto() (null elements become NULL, not the primitive default) and are visibly different through getInt64Array().
  • equals/hashCode contract violationPrimitiveArrayImpl.valueHash() does include the null mask (31 * Objects.hashCode(nulls) + arrayHash()), so values that compared equal hashed differently, breaking HashMap/HashSet usage of Value, Struct, and Mutation.
  • Wrong mutation diffs — the Cloud Spanner Import/Export Dataflow template relies on Mutation.equals to 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 valueEquals override on PrimitiveArrayImpl, mirroring the existing valueHash()/arrayHash() split, and turn the per-subclass valueEquals overrides into arrayEquals.

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>
@fornwall
fornwall requested review from a team as code owners July 18, 2026 10:45

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant