mandrean opened a new pull request, #3520:
URL: https://github.com/apache/fory/pull/3520
## 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
