Skip to content

[api][java] Include the memory type in MemoryRef equality and reject null fields - #1153

Merged
wenjin272 merged 2 commits into
apache:mainfrom
sangkyoonnam:fix/memory-ref-equality
Sep 28, 2026
Merged

wenjin272 merged 2 commits into
apache:mainfrom
sangkyoonnam:fix/memory-ref-equality

Conversation

@sangkyoonnam

@sangkyoonnam sangkyoonnam commented Sep 23, 2026 •

Copy link
Copy Markdown
Contributor

Linked issue: #1152

Purpose of change

User-visible outcome

Two MemoryRef values are now equal only when both the memory type and the path are equal; hashCode and toString follow. MemoryRef.create rejects a null type or path with an IllegalArgumentException. Previously a sensory and a short-term reference to the same path compared equal, hashed together and printed identically, and a null type surfaced later as a NullPointerException from resolve or from the Jackson serializer.

Intent

A reference names a slot in one memory. equals was written when the class had only a path (#110); the sensory-memory change added the type without revisiting it. The Python MemoryRef is a frozen pydantic model and already compares and hashes both fields, so this aligns the Java side with it.

Runtime flow

No call path changes. MemoryObjectImpl#get(MemoryRef) and EventAttachmentUtils read the type and path through getters and are unaffected. The JSON form already carries memory_type, so the existing EventTest round-trip assertion still holds.

Key decisions

  • Keep MemoryObject#get(MemoryRef) resolving by path only, in both languages. Its contract says it "represents the path of the given reference"; whether a reference should be rejected when resolved against the other memory is a separate question.
  • IllegalArgumentException for a null type or path, matching the class's own Deserializer and the other recent argument checks in api.

Behavioral Semantics

Interaction decisions

Equality is type == other.type && path.equals(other.path); hashCode is Objects.hash(type, path); toString prints both. create checks the type first, then the path.

Behavioral contracts

  1. References with different memory types and the same path are not equal.
  2. References with the same memory type and path are equal and share a hash code (unchanged).
  3. toString names the memory type and the path.
  4. create(null, path) and create(type, null) raise IllegalArgumentException.

Failure behavior

The only new failure path is create with a null argument, which now throws at construction; nothing is created, logged or absorbed. The NullPointerException a null type used to cause in resolve and in serialization is no longer reachable.

Tests

Contracts to tests

Contract Tests
1 MemoryRefTest.testRefEqualityDistinguishesMemoryType
2 MemoryRefTest.testRefEqualityAndHashing (pre-existing)
3 MemoryRefTest.testToStringNamesMemoryType
4 MemoryRefTest.testCreateRejectsNullTypeOrPath

Coverage and what was not verified

mvn -pl api install && mvn -pl runtime test: 983, 0 failures. mvn -pl api,plan test: 454 and 397, 0 failures. spotless:check passes. Without the MemoryRef change exactly the three new tests fail.

The Python side was checked directly against flink_agents.api.memory_reference: a sensory and a short-term reference to the same path are not equal, a set of both keeps two entries, repr shows the memory type, and create(None, path) raises ValidationError. The Python model is unchanged by this PR.

Not verified: no production code in api, plan or runtime keeps references in a Set or Map today, so the collection behavior is covered at the unit level only.

Implementation invariants and supporting evidence
  • type is an enum, so identity comparison is exact.
  • The Deserializer already rejects a missing memory_type or path, so create now matches the JSON path.
  • The pre-existing testRefEqualityAndHashing uses SHORT_TERM only, which is why it did not catch this.

API

Compatibility impact

MemoryRef is public API. Only code that relied on two references of different types comparing equal changes behavior; no such use exists in the repository. create now throws on a null type or path instead of deferring the failure. Python is unchanged.

Documentation

  • doc-needed
  • doc-not-needed
  • doc-included

Was this patch authored or co-authored using generative AI tooling?

  • Yes
  • No

Generated-by: Claude Code 2.1.280 (Claude Fable 5.1)

@github-actions github-actions Bot added doc-not-needed Your PR changes do not impact docs fixVersion/0.4.0 priority/major Default priority of the PR or issue. labels Sep 23, 2026
@sangkyoonnam
sangkyoonnam force-pushed the fix/memory-ref-equality branch from 4393a28 to 2699cd4 Compare September 23, 2026 19:00
@sangkyoonnam

sangkyoonnam commented Sep 25, 2026 •

Copy link
Copy Markdown
Contributor Author

One correction. The description said references with different memory types do not share a hash code, and the test asserted it. Objects.hash(type, path) does not guarantee that: enum hash codes are identity-based. The description was already corrected; fd893b7 drops the assertion and the test comment that promised distinct hash buckets, keeps the check that equal references share a hash, and keeps the pre-existing check on two fixed paths of the same type, which is deterministic. An earlier push of this correction removed that pre-existing check instead; fd893b7 replaces it.

@github-actions github-actions Bot added doc-not-needed Your PR changes do not impact docs and removed doc-not-needed Your PR changes do not impact docs labels Sep 25, 2026
@sangkyoonnam
sangkyoonnam force-pushed the fix/memory-ref-equality branch from 81c2e1f to fd893b7 Compare September 25, 2026 06:36
…null fields

Generated-by: Claude Code 2.1.280 (Claude Fable 5.1)
Generated-by: Claude Code 2.1.280 (Claude Fable 5.1)
@sangkyoonnam
sangkyoonnam force-pushed the fix/memory-ref-equality branch from fd893b7 to 5a9c6e9 Compare September 28, 2026 04:30

@wenjin272 wenjin272 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.

Thanks for taking this on. LGTM

@wenjin272
wenjin272 merged commit cc238a1 into apache:main Sep 28, 2026
28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

doc-not-needed Your PR changes do not impact docs fixVersion/0.4.0 priority/major Default priority of the PR or issue.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants