[GitHub] [iceberg] kbendick opened a new issue #1250: Some compiler warnings about ReferenceEquality and potentially fragile code

[GitBox <gi...@apache.org>]

kbendick opened a new issue #1250:
URL: https://github.com/apache/iceberg/issues/1250


   When building the project from a clean download using `./gradlew build`, there are several warnings about class methods that are using reference equality (e.g. `==`) for comparison instead of value equality.
   
   There are almost certainly places where reference equality is intended. It's also possible that hidden bugs exist or could come into existence later during some refactor. The `Error Prone` linter that we use marks this warning as `Fragile Code`.
   
   I would propose that these carefully be looked at and either fixing them to use some form of value equality or documenting false positives via `SuppressWarnings("ReferenceEquality")` or similar annotations. For new comers to the project, I think that adding the appropriate annotation would really help in readability and understanding what's happening at the physical layer for comparisons.
   
   Here is an example
   ```
   /Users/kylebendickson/repos/iceberg/api/src/main/java/org/apache/iceberg/types/TypeUtil.java:46: warning: [ReferenceEquality] Comparison using reference equality instead of value equality
       if (schema.asStruct() == result) {
                             ^
       (see https://errorprone.info/bugpattern/ReferenceEquality)
     Did you mean 'if (Objects.equals(schema.asStruct(), result)) {' or 'if (schema.asStruct().equals(result)) {'?
   ```
   
   I'd be happy to submit a PR to remove some of these warnings or to at least open a discussion to switching these warnings to use value equality to allow safer refactoring of code and easier understanding of what's happening with iceberg types when comparing.
   
   Here's a link to the error prone documentation on this warning: https://errorprone.info/bugpattern/ReferenceEquality


----------------------------------------------------------------
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.

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



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org

[GitHub] [iceberg] rdblue commented on issue #1250: Some compiler warnings about ReferenceEquality and potentially fragile code

[GitBox <gi...@apache.org>]

rdblue commented on issue #1250:
URL: https://github.com/apache/iceberg/issues/1250#issuecomment-664051713


   It would be great to start suppressing the warnings. Most (hopefully all) of the places where we do this are intended to catch the original object, as in the `TypeUtil` example you pasted that is checking whether projecting fields returned the whole original schema.


----------------------------------------------------------------
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.

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



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org

[GitHub] [iceberg] kbendick commented on issue #1250: Some compiler warnings about ReferenceEquality and potentially fragile code

[GitBox <gi...@apache.org>]

kbendick commented on issue #1250:
URL: https://github.com/apache/iceberg/issues/1250#issuecomment-667441829


   Yes. For this one I imagine that most of them, if not all, will be suppressions. I will open a PR for this sometime this weekend or early next week. And then hopefully we can add a github action that blocks PRs on certain ErrorProne cases if people are open to it (at least for important things like ReferenceEquality).
   
   The @override thing wasn't really a big deal, but over time the Suppress Warning on things like reference equality will become more and more important as forms of documentation for the reader.
   
   Whether or not we want to block PRs yet is a separate issue.


----------------------------------------------------------------
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.

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



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org