[PR] Compare Go set and entry-slice map fields without regard to order [thrift]

["slachiewicz (via GitHub)" <gi...@apache.org>]

slachiewicz opened a new pull request, #3805:
URL: https://github.com/apache/thrift/pull/3805

   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
   
   - **No JIRA ticket yet.** I could not file one. Happy to add the key to the title and the branch name once it exists; the description above is the ticket text.
   - Opened as a draft until the ticket exists and CI is green.
   - 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: notifications-unsubscribe@thrift.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Compare Go set and entry-slice map fields without regard to order [thrift]

["slachiewicz (via GitHub)" <gi...@apache.org>]

slachiewicz commented on PR #3805:
URL: https://github.com/apache/thrift/pull/3805#issuecomment-5561099153

   Superseded by #3806. GitHub closed this one when the branch was renamed to THRIFT-6195 to match the ticket, which had not been filed when this was opened. Same commit, same content.
   
   *This comment 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: notifications-unsubscribe@thrift.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Compare Go set and entry-slice map fields without regard to order [thrift]

["slachiewicz (via GitHub)" <gi...@apache.org>]

slachiewicz closed pull request #3805: Compare Go set and entry-slice map fields without regard to order
URL: https://github.com/apache/thrift/pull/3805


-- 
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: notifications-unsubscribe@thrift.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org