[jira] [Comment Edited] (THRIFT-5483) Support customized comparator in Java

["Liu Jiayu (Jira)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/THRIFT-5483?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17522058#comment-17522058 ] 

Liu Jiayu edited comment on THRIFT-5483 at 4/14/22 4:40 AM:
------------------------------------------------------------

OK in this case your issue isn't related to comparator, but instead is how one can use customized equals and hashCode function, for (generated) classes that you don't own.

This is an issue that affects many frameworks and arguably is a historical burden tie to Java itself. A better design would be to _not_ to have *one* hashCode definition for all any Object, but allow HashMap to take in a separate supplier, like what Rust did in [https://doc.rust-lang.org/std/hash/index.html|https://doc.rust-lang.org/std/hash/index.html.]

My suggestion, knowing that this isn't something that can be fixed from within Thrift, is to _not_ to use Thrift generated class as Map keys at all, given that in may cases it's not a good practice after all:
 * map keys should be immutable but thrift generated structs are not
 * map keys should be having as small footprints as possible but the thrift generate classes take in all fields by default

I think you should do what [~emmenlau] had suggested, use a class that's defined and controlled by yourself as key. To have better encapsulation i'd even hide way map implementation:
{code:java}
interface SupervisorLookup {
  Optional<Person> findSupervisor(Person person);
}

class DefaultSupervisorLookup {
  private final Map<Long, Person> personIdToSupervisor;

  Optional<Person> findSupervisor(Person person) {
    return Optional.ofNullable(personIdToSupervisor(person.getId());
  }
}{code}


was (Author: jiayuliu):
OK in this case your issue isn't related to comparator, but instead is how one can use customized equals and hashCode function, for (generated) classes that you don't own.

This is an issue that affects many frameworks and arguably is a historical burden tie to Java itself. A better design would be to _not_ to have *one* hashCode definition for all any Object, but allow HashMap to take in a separate supplier, like what Rust did in [https://doc.rust-lang.org/std/hash/index.html.]

My suggestion, knowing that this isn't something that can be fixed from within Thrift, is to _not_ to use Thrift generated class as Map keys at all, given that in may cases it's not a good practice after all:
 * map keys should be immutable but thrift generated structs are not
 * map keys should be having as small footprints as possible but the thrift generate classes take in all fields by default

I think you should do what [~emmenlau] had suggested, use a class that's defined and controlled by yourself as key. To have better encapsulation i'd even hide way map implementation:
{code:java}
interface SupervisorLookup {
  Optional<Person> findSupervisor(Person person);
}

class DefaultSupervisorLookup {
  private final Map<Long, Person> personIdToSupervisor;

  Optional<Person> findSupervisor(Person person) {
    return Optional.ofNullable(personIdToSupervisor(person.getId());
  }
}{code}

> Support customized comparator in Java
> -------------------------------------
>
>                 Key: THRIFT-5483
>                 URL: https://issues.apache.org/jira/browse/THRIFT-5483
>             Project: Thrift
>          Issue Type: Improvement
>          Components: Java - Compiler
>            Reporter: Tian Jiang
>            Priority: Trivial
>         Attachments: screenshot-1.png
>
>
> The generated comparator (and hash code) of Thrift Structs compares two objects by all of their fields, which may cause trouble when used in hash structures.
> For example, I define a struct like:
> {code:java}
> struct Person{
>   1: required i64 id
>   2: required string phoneNumber
> }
> {code}
> The id of a Person is final, while its phoneNumber can be changed.
> Then I use it as a key in a map to record a Person's supervisors:
> {code:java}
> Map<Person, Person> supervisorMap;
> {code}
> Apparently, as the generated comparator of Person will use both id and phoneNumber for comparison and hash if I change the phoneNumber of a Person, I can no longer get it from the map as its hash value changes and it will be distributed to another hash slot.
> Therefore, I wish I could specify the fields that are to be used in comparator and hash code generation. One preferable grammar may be like:
> {code:java}
> struct Person{
>   1: required comparable i64 id
>   2: required string phoneNumber
> }
> {code}
> Then the generated comparator and hash code will only use fields marked with `comparable`, and if no fields are marked with comparable, Java's default comparison and hashcode (using object address) will be used. 



--
This message was sent by Atlassian Jira
(v8.20.1#820001)