AVRO-3503 Fix usage of 'is' and 'as' in Equals

[KyleSchoonover]

Jira

• My PR addresses the following Avro Jira issues and references them in the PR title. For example, "AVRO-1234: My Avro PR"
  • https://issues.apache.org/jira/browse/AVRO-3503

Tests

• My PR adds the following unit tests: Adding coverage for Equals changes

Commits

• My commits all reference Jira issues in their subject lines. In addition, my commits follow the guidelines from "How to write a good git commit message":
  1. Subject is separated from body by a blank line
  2. Subject is limited to 50 characters (not including Jira issue reference)
  3. Subject does not end with a period
  4. Subject uses the imperative mood ("add", not "adding")
  5. Body wraps at 72 characters
  6. Body explains "what" and "why", not "how"

Documentation

• In case of new functionality, my PR adds documentation that describes how to use it.
  • All the public functions and the classes in the PR contain Javadoc that explain what it does

[KyleSchoonover]

Currently working on tests.

[zcsizmadia]

I recommend to add the unit tests first into a different PR for each Equals code which will be changed. Once there is test coverage, the Equals code should be modified in a seperate PR.

[zcsizmadia]

I am not sure changing to SequenceEqual is the way to go. IMO those changes have a high risk for a breaking change.

[KyleSchoonover]

I can break it out into separate PR's. I'm going to use this as a bucket to check against the CI pipelines.

As for SequenceEqual, I find it better than the for loops and nested if statements as long as it is comparing simple types. I'm reluctant to use it for complex types.

[zcsizmadia]

The complexity of reviewing the SequenceEqual change is definetely higher than the actual problem the ticket scopes for. E.g. SequenceEqual is using the default comparer EquityComparer<string>.Default. Which is in your case is the Ordinal comparison code. IMO that knowledge is not trivial and can be easily missed.

The unit tests should provide the baseline for the changes you make, unless the changes are trivial.

[zcsizmadia]

The other issue is changing the code behaviour of the Equals functions. It is standard practice to either throwing a exception if the argument is null or returning false. Some of the changes would return false instead of throwing the exception (which is the current implementation in some of those codes). Which might be ok, if we mark this PR as a major release PR, but in that case, the scope is definetely bigger than it is described in the ticket.

[KalleOlaviNiemitalo]

In general, I'd recommend having A.Equals(object obj) compare obj.GetType() == this.GetType() rather than obj.GetType() == typeof(A). This way, if another class B is derived from A, then B.Equals(object obj) can call base.Equals(obj) to compare the types and the members inherited from A. If A is a struct or a sealed class, then the JIT compiler should be able to optimize this.GetType() to typeof(A).