slachiewicz opened a new pull request, #3806:
URL: https://github.com/apache/thrift/pull/3806
Fixes THRIFT-6195.
Thrift defines `set` and `map` as unordered. The Go binding represents a
`set`, and a `map` whose key type cannot be a Go map key, as a slice, and the
generated `Equals` compared those slices position by position. Two values
holding the same members in a different order therefore compared unequal.
This affects three constructs, because they share one code path in
`generate_go_equals_container`:
| Thrift type | Go type | Since |
|---|---|---|
| `set<T>` | `[]T` | long-standing |
| `map<container, V>` | `[]thrift.MapEntry[K, V]` | THRIFT-2063 |
| `map<struct, V>` with `go:struct_key_entries` | `[]thrift.MapEntry[*K, V]`
| THRIFT-6175, #3788 |
Java compares these with `HashSet` and `HashMap` equality and Python with
`set` and `dict`, so both are order-insensitive without extra work. Go cannot
use a native map or set here, because a slice, a map, or a struct holding
either is not a valid Go map key. The slice representation is forced by the
language; the positional comparison is not.
### What changed
Sets and entry slices now go through a new `thrift.UnorderedEqual`. Lists
keep the positional comparison, because a list is ordered, so the list and set
arm is split.
The helper compares position by position first and only then falls back to
matching each element against an unmatched element of the other side. Values
that agree on order, which includes anything that has just been deserialized,
cost one pass and allocate nothing.
Matching consumes an element of the other side at most once. Without that a
set holding `{a, a}` compares equal to one holding `{a, b}`, since both copies
of `a` match the same element. `TestUnorderedEqual` pins that case and the
symmetric one.
### Cost
The fallback is quadratic. A hashed index would make it linear for element
types that can be Go map keys, but it needs a type predicate, a separate nil
path, and per-type projections, and it is roughly twelve times slower than the
positional pass on the common path when applied unconditionally. Trying
positional first avoids paying that, and the hashed tier can be added later if
a profile asks for it.
`TestUnorderedEqualOrderedPathDoesNotAllocate` pins the zero-allocation
property of the fast path.
### Compatibility
This changes observable behaviour. Code whose tests depend on positional
comparison of a set or an entry-slice map field will start seeing values
compare equal that previously did not. Nothing semantically correct should
break, since the change only makes `Equals` agree with what the Thrift type
system already says these collections mean.
### Notes for the reviewer
- Opened as a draft until CI is green.
- Supersedes #3805, which GitHub closed when its branch was renamed to match
the ticket.
- Overlaps in spirit with #3788, but the two are independent and touch
different functions. Once both land, struct-keyed maps generated with
`go:struct_key_entries` pick this up automatically.
- The `-remote` packages for `includestest` fail to build on master already,
with `undefined: includestest.Numberz`. That is unrelated and not part of the
`check` target.
*This change was created with AI assistance.*
--
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]
