Skip to content

fix(java): support TreeSet/TreeMap subclasses without Comparator constructor in SortedSet/SortedMapSerializer#3344

Merged
chaokunyang merged 2 commits intoapache:mainfrom
mandrean:treeset-subclass-comparator
Feb 17, 2026
Merged

fix(java): support TreeSet/TreeMap subclasses without Comparator constructor in SortedSet/SortedMapSerializer#3344
chaokunyang merged 2 commits intoapache:mainfrom
mandrean:treeset-subclass-comparator

Conversation

@mandrean
Copy link
Copy Markdown
Contributor

@mandrean mandrean commented Feb 16, 2026 •
edited
Loading

Why?

SortedSetSerializer and SortedMapSerializer unconditionally require a (Comparator) constructor for all TreeSet/TreeMap subclasses. Java constructors are not inherited, so a class ChildTreeSet extends TreeSet<String> (or class ChildTreeMap extends TreeMap<String, String>) with only a no-arg constructor throws UnsupportedOperationException when registered with the corresponding serializer.

What does this PR do?

Changes both SortedSetSerializer and SortedMapSerializer to try the (Comparator) constructor first, and fall back to the no-arg constructor if not found. At deserialization time, uses whichever constructor is available. The no-arg fallback uses natural ordering, which is the common case for TreeSet/TreeMap subclasses.

Only throws UnsupportedOperationException if neither constructor is found.

Related issues

Does this PR introduce any user-facing change?

  • Does this PR introduce any public API change?
    No. Existing behavior is preserved for classes that have a (Comparator) constructor. Classes that previously threw UnsupportedOperationException now work.

  • Does this PR introduce any binary protocol compatibility change?
    No. The write path is unchanged (size + comparator ref), and the read path still reads the same bytes in the same order. Only the post-read instantiation strategy differs (which constructor is used). Wire format is identical in both directions.

Benchmark

Not applicable; the constructor lookup happens once at serializer construction time, not per serialization. The runtime paths (newCollection/newMap) add a single null check (if (comparatorConstructor != null)) which is negligible.

@mandrean mandrean force-pushed the treeset-subclass-comparator branch from db9b3e4 to 95a5eed Compare February 16, 2026 12:48
@chaokunyang
Copy link
Copy Markdown
Collaborator

chaokunyang commented Feb 17, 2026

It seems that the comparator is null if the treeset subclass don't pass comparator to parent class. In such cases, treeset Or treemap will just use key compareTo api for sort. So it's not a bug, could you remove the log and keep constructor branch. And could you ask check TreeMap? I think it has similar issue

@mandrean mandrean force-pushed the treeset-subclass-comparator branch from 95a5eed to ecf8f6d Compare February 17, 2026 08:26
@mandrean mandrean changed the title fix(java): Support TreeSet subclasses without Comparator constructor in SortedSetSerializer fix(java): support TreeSet/TreeMap subclasses without Comparator constructor in SortedSet/SortedMapSerializer Feb 17, 2026
@mandrean
Copy link
Copy Markdown
Contributor Author

mandrean commented Feb 17, 2026

Thanks, pushed some changes, have a look again @chaokunyang!

@mandrean mandrean requested a review from chaokunyang February 17, 2026 08:34
chaokunyang
chaokunyang approved these changes Feb 17, 2026
View reviewed changes
Copy link
Copy Markdown
Collaborator

@chaokunyang chaokunyang left a comment

Choose a reason for hiding this comment

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

Looks great!

@chaokunyang chaokunyang merged commit 4f4453d into apache:main Feb 17, 2026
58 checks passed
This was referenced Mar 26, 2026
Open
Open
Closed
Merged
Open
Open
mandrean added a commit to mandrean/fory that referenced this pull request Mar 26, 2026
@mandrean
fix(java): auto-select optimized serializers for sorted container sub…
75e6e44 
…classes

Adds optimized child-serializers for subclasses of TreeSet,
ConcurrentSkipListSet, PriorityQueue, TreeMap, and ConcurrentSkipListMap.
Previously these fell back to JDK-compatible serialization because the
hierarchy walk hit writeObject/readObject on the parent JDK type.

The new serializers handle comparator preservation, slot field
serialization, and ref-tracking (including the deferred-ref pattern for
concurrent variants).

Constructor discovery is centralized in ContainerConstructors, which
probes for all standard constructor shapes and gracefully falls through
to natural ordering when no comparator-preserving constructor is
available (per apache#3344 (comment)).

Existing SortedSetSerializer, SortedMapSerializer, and
PriorityQueueSerializer are refactored to reuse the same factory logic.
mandrean added a commit to mandrean/fory that referenced this pull request Mar 26, 2026
@mandrean
test(java): add tests for sorted container subclass auto-selection
c8f8eb6 
Covers all constructor variants for TreeSet, ConcurrentSkipListSet,
PriorityQueue, TreeMap, and ConcurrentSkipListMap child classes:
- (Comparator), (SortedSet)/(SortedMap) preserve comparator
- (), (Collection)/(Map), (int) use natural ordering
- Unsupported constructors and custom-serialized children fall back
- Async compilation path verified
- InternalComparatorTreeSet verifies the graceful fall-through from
  apache#3344 (comment)

Also adds (Collection) and (SortedSet)/(SortedMap) constructor tests
for the existing SortedSetSerializer and SortedMapSerializer.
mandrean added a commit to mandrean/fory that referenced this pull request Mar 26, 2026
@mandrean
fix(java): auto-select optimized serializers for sorted container sub…
0cf4935 
…classes

Adds optimized child-serializers for subclasses of TreeSet,
ConcurrentSkipListSet, PriorityQueue, TreeMap, and ConcurrentSkipListMap.
Previously these fell back to JDK-compatible serialization because the
hierarchy walk hit writeObject/readObject on the parent JDK type.

The new serializers handle comparator preservation, slot field
serialization, and ref-tracking (including the deferred-ref pattern for
concurrent variants).

Constructor discovery is centralized in ContainerConstructors, which
probes for all standard constructor shapes and gracefully falls through
to natural ordering when no comparator-preserving constructor is
available (per apache#3344 (comment)).

Existing SortedSetSerializer, SortedMapSerializer, and
PriorityQueueSerializer are refactored to reuse the same factory logic.
mandrean added a commit to mandrean/fory that referenced this pull request Mar 26, 2026
@mandrean
test(java): add tests for sorted container subclass auto-selection
10d08df 
Covers all constructor variants for TreeSet, ConcurrentSkipListSet,
PriorityQueue, TreeMap, and ConcurrentSkipListMap child classes:
- (Comparator), (SortedSet)/(SortedMap) preserve comparator
- (), (Collection)/(Map), (int) use natural ordering
- Unsupported constructors and custom-serialized children fall back
- Async compilation path verified
- InternalComparatorTreeSet verifies the graceful fall-through from
  apache#3344 (comment)

Also adds (Collection) and (SortedSet)/(SortedMap) constructor tests
for the existing SortedSetSerializer and SortedMapSerializer.
mandrean added a commit to mandrean/fory that referenced this pull request Mar 26, 2026
@mandrean
test(java): add tests for sorted container subclass auto-selection
95a441f 
Covers all constructor variants for TreeSet, ConcurrentSkipListSet,
PriorityQueue, TreeMap, and ConcurrentSkipListMap child classes:
- (Comparator), (SortedSet)/(SortedMap) preserve comparator
- (), (Collection)/(Map), (int) use natural ordering
- Unsupported constructors and custom-serialized children fall back
- Async compilation path verified
- InternalComparatorTreeSet verifies the graceful fall-through from
  apache#3344 (comment)

Also adds (Collection) and (SortedSet)/(SortedMap) constructor tests
for the existing SortedSetSerializer and SortedMapSerializer.
chaokunyang added a commit that referenced this pull request Mar 27, 2026
@mandrean @chaokunyang
fix(java): preserve ConcurrentSkipListSet comparator on copy (#3520)
14f9f4d 
## Why?

As a follow-up to #3342 and #3344, I wanted to make sure there is test
coverage for all the public constructors of the different Map/Set
classes supported by the serializers.

That exposed a copy-path bug in `ConcurrentSkipListSetSerializer`
(reported as #3519) which this PR addresses.

The deserialize path already preserves the comparator, but
`fory.copy(...)` rebuilt `ConcurrentSkipListSet` with the default
constructor path, which dropped the comparator and silently changed
ordering semantics. This is easy to miss because `Set.equals(...)` still
passes even when iteration order changes.

## What does this PR do?

Its kind-of two part: fix the bug, and add more test coverage.
Specifically:

- Preserve the comparator when copying `ConcurrentSkipListSet` by
overriding the collection copy construction path in
`ConcurrentSkipListSetSerializer`.
- Add constructor-matrix coverage for:
  - `TreeSet`
  - `TreeMap`
  - `ConcurrentSkipListSet`
  - `ConcurrentSkipListMap`
  - `PriorityQueue`
- Exercise both round-trip deserialize and `fory.copy(...)` behavior for
the relevant public constructors.
- Assert concrete runtime type, comparator nullability, and observable
ordering semantics.
- Use queue-specific assertions for `PriorityQueue` by comparing drained
poll order instead of relying on `equals(...)`.

## Related issues

- Fixes #3519
- Related to #3337
- Related to #3343

## AI Contribution Checklist

- [x] Substantial AI assistance was used in this PR: `yes` / `no`
- [x] If `yes`, I included a completed [AI Contribution
Checklist](https://github.com/apache/fory/blob/main/AI_POLICY.md#9-contributor-checklist-for-ai-assisted-prs)
in this PR description and the required `AI Usage Disclosure`.
- [x] If `yes`, I can explain and defend all important changes without
AI help.
- [x] If `yes`, I reviewed AI-assisted code changes line by line before
submission.
- [x] If `yes`, I ran adequate human verification and recorded evidence
(checks run locally or in CI, pass/fail summary, and confirmation I
reviewed results).
- [x] If `yes`, I added/updated tests and specs where required.
- [x] If `yes`, I validated protocol/performance impacts with evidence
when applicable.
- [x] If `yes`, I verified licensing and provenance compliance.

AI Usage Disclosure
- substantial_ai_assistance: yes
- scope: code drafting, test design
- affected_files_or_subsystems: java/fory-core collection serializers
and java/fory-core collection/map serializer tests
- human_verification: local verification evidence available: `cd java &&
ENABLE_FORY_DEBUG_OUTPUT=1 mvn -T16 -pl fory-core
-Dtest=org.apache.fory.serializer.collection.CollectionSerializersTest,org.apache.fory.serializer.collection.MapSerializersTest
test` -> pass (237 tests, 0 failures)
- performance_verification: no dedicated benchmark run; runtime change
is limited to the `fory.copy(...)` path for `ConcurrentSkipListSet`,
while serialize/deserialize hot paths are unchanged; the added
comparator copy matches existing `PriorityQueue` and
`ConcurrentSkipListMap` copy behavior
- provenance_license_confirmation: repository-local
Apache-2.0-compatible changes only; no third-party code introduced

## Does this PR introduce any user-facing change?

- [ ] Does this PR introduce any public API change?
- [ ] Does this PR introduce any binary protocol compatibility change?

## Benchmark

No dedicated benchmark was run.

This PR is a correctness fix plus test coverage expansion, not a
performance optimization:
- The runtime code change only affects the `fory.copy(...)` path for
`ConcurrentSkipListSet`.
- Serialization and deserialization hot paths are unchanged.
- The added work is one comparator copy per copied
`ConcurrentSkipListSet`, which is expected to be negligible relative to
element copy and skip-list insertion work, and is consistent with
existing `PriorityQueue` and `ConcurrentSkipListMap` copy behavior.

Co-authored-by: Shawn Yang <shawn.ck.yang@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@chaokunyang chaokunyang chaokunyang approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Java] SortedSet/SortedMapSerializer fails for TreeSet/TreeMap subclasses without Comparator constructor

2 participants

@mandrean @chaokunyang