Skip to content

[common] Fix equals/hashCode contract in value and kv record batches - #3952

Open
vbhanuchander-lang wants to merge 1 commit into
apache:mainfrom
vbhanuchander-lang:record-batch-hashcode
Open

[common] Fix equals/hashCode contract in value and kv record batches#3952
vbhanuchander-lang wants to merge 1 commit into
apache:mainfrom
vbhanuchander-lang:record-batch-hashcode

Conversation

@vbhanuchander-lang

Copy link
Copy Markdown

Purpose

Linked issue: close #3951

DefaultValueRecordBatch and DefaultKvRecordBatch override equals() with value
semantics but never override hashCode(). Both classes only implements their interface,
so their superclass is Object, and neither equals() delegates to super.equals()
hashCode() is therefore the identity hash. Two byte-identical batches are equals() but
return different hash codes, which violates the general contract of Object.hashCode().

The third batch implementation in the same package, DefaultLogRecordBatch, has the
identical equals() shape and does hash the same byte range
(DefaultLogRecordBatch.java:316),
which is why this reads as an oversight rather than a deliberate choice. The Checkstyle
EqualsHashCode module is not enabled in tools/maven/checkstyle.xml, so CI does not flag
it today.

Brief change log

  • DefaultValueRecordBatch: add hashCode(), mirroring DefaultLogRecordBatch exactly.
  • DefaultKvRecordBatch: same.

Both use MurmurHashUtils.hashBytes(segment, position, sizeInBytes()). equals() compares
the byte range [position, position + sizeInBytes()) via
segment.equalTo(that.segment, position, that.position, sizeInBytes), and hashBytes
hashes exactly that range, so equals() and hashCode() are consistent by construction.

Tests

New in DefaultValueRecordBatchTest (new file) and DefaultKvRecordBatchTest:

Test Asserts
testEqualsAndHashCode two independently built, byte-identical batches are equal and share a hash code (fails before this change)
testHashCodeDiffersForDifferentContents batches with different contents are unequal and hash differently
testEmptyBatchesAreEqualAndShareHashCode (value batch only) the empty-batch case, which is the shape asserted in ReplicaManagerTest#testLimitScanPrimaryKeyTable

Verified locally with mvn clean verify -pl fluss-common: 1748 + 186 tests pass, Checkstyle
reports 0 violations, spotless:check and apache-rat:check pass.

API and Format

No API or storage-format change. hashCode() is additive.

The only behavioural change is for a HashSet/HashMap keyed on one of these batches,
which would previously have used the identity hash. Nothing does that today — the batches
appear only as map value types (e.g. Map<TableBucket, KvRecordBatch> in
ServerRpcMessageUtils#getPutKvData and ReplicaManager#putRecordsToKv), where
hashCode() is never consulted.

Documentation

No new feature, no documentation change.

Note on #3877

I found this while diagnosing the flaky ReplicaManagerTest#testLimitScanPrimaryKeyTable
in #3877. This PR does not fix that flakiness — why the two batches genuinely differ
there is a separate question. It does make the failure message more useful: neither class
overrides toString(), so Object.toString() renders the identity hash, and the
...@60eebc2e vs ...@1408822 in that CI log cannot tell a reader whether the contents
actually differ (two equal batches would also print different suffixes). With a
content-derived hashCode(), differing suffixes do mean differing bytes.

Generative AI disclosure

  • Yes — Claude Code (Opus 5). All changes reviewed by me before submitting.

DefaultValueRecordBatch and DefaultKvRecordBatch override equals() with
value semantics but do not override hashCode(). Both extend Object and
neither equals() delegates to super.equals(), so hashCode() is the
identity hash: two byte-identical batches are equal but hash differently,
violating the general contract of Object.hashCode().

The third batch implementation in the same package,
DefaultLogRecordBatch, has the identical equals() shape and already
hashes the same byte range. Mirror it in both classes so the three
siblings stay consistent.

This is not a fix for the flakiness in apache#3877; it only removes the
contract violation observed while diagnosing it.
@vbhanuchander-lang

Copy link
Copy Markdown
Author

Could a committer approve the pending workflow runs? All three (CI, Check License,
Client Integration) are at action_required, so nothing has executed on this branch yet.
Locally mvn clean verify -pl fluss-common is green — 1748 + 186 tests, 0 Checkstyle
violations — and spotless:check and apache-rat:check pass.

cc @loserwang1024 @luoyuxia — you are the most recent contributors to these files. The change is
four lines of production code: DefaultValueRecordBatch and DefaultKvRecordBatch override
equals() with value semantics but never override hashCode(), so byte-identical batches are
equal while hashing differently. DefaultLogRecordBatch in the same package already has the
identical equals() shape plus the matching hashCode(), and I mirrored it exactly rather than
inventing anything.

Details, including why nothing currently depends on the old identity hash, are in #3951.

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.

[common] DefaultValueRecordBatch and DefaultKvRecordBatch override equals() without hashCode()

1 participant