search

apache / iceberg

Apache Iceberg
https://iceberg.apache.org/
Apache License 2.0
6.49k stars 2.24k forks source link

Review new ImmutablesReferenceEquality error-prone check #10855

Open findepi opened 3 months ago

findepi commented 3 months ago

ImmutablesReferenceEqualityis a recently added error-prone check (https://github.com/apache/iceberg/pull/10788). Figure out whether we adjust the code or suppress the check permanently

danielhumanmod commented 3 months ago

Hi team, may I take a look on this issue?

findepi commented 3 months ago

sure, go for it!

danielhumanmod commented 3 months ago

Hi @findepi , based on my investigation, the current status regarding the usage of ImmutablesReferenceEquality is as follows:

Currently, ImmutablesReferenceEquality is only triggered in line 119 of BaseViewOpertaions.java

// if the metadata is not changed, return early
    if (base == metadata) {
      LOG.info("Nothing to commit.");
      return;
    }

According to PR #10899 ,this comparison is intentionally performing a reference equality check. Therefore, the @SuppressWarnings("ImmutablesReferenceEquality") here is reasonable.

Considering for most cases we don't encourage reference equality check for immutable objects, we should consider keep this error-prone check. For cases where reference equality is indeed necessary, contributors can use @SuppressWarnings("ImmutablesReferenceEquality") to bypass the warning. However, contributors should explain the necessity of the reference equality check, and all instances of @SuppressWarnings should be carefully reviewed during code reviews.

danielhumanmod commented 3 months ago

Additionally, I do notice that there are several DangerousJavaDeserialization warning in our code, do we have a plan to do some investigation on that?

danielhumanmod commented 2 months ago

Hi @findepi , I’ve shared an initial conclusion in my previous comment regarding the usage of ImmutablesReferenceEquality. Do you think any further action is needed on this issue, or should we involve additional reviewers?

Thanks for your input!

findepi commented 2 months ago

thanks for your insights @danielhumanmod !

there are several DangerousJavaDeserialization warning in our code, do we have a plan to do some investigation on that?

There is a separate issue about this: https://github.com/apache/iceberg/issues/10853

Currently, ImmutablesReferenceEquality is only triggered in line 119 of BaseViewOpertaions.java

if this is the only place, then yes it makes sense to enable the check and just suppress in this place.

And yes, it's intentional.

However, contributors should explain the necessity of the reference equality check, and all instances of @SuppressWarnings should be carefully reviewed during code reviews.

@danielhumanmod good point. Suppression should be last resort and code-commented.