Build: Fix errorprone warnings

[ajantha-bhat]

Apache Iceberg version

None

Query engine

None

Please describe the bug 🐞

./gradlew clean build -x test -x integrationTest --no-build-cache

Default profile build with above command results in few errorprone warnings.

Good to keep the build green.

Willingness to contribute

• I can contribute a fix for this bug independently
• I would be willing to contribute a fix for this bug with guidance from the Iceberg community
• I cannot contribute a fix for this bug at this time

[ajantha-bhat]

Good first issue for new contributors.

[TomasDarquier]

• I can contribute a fix for this bug independently
  Hi, I’d like to work on this issue — it seems like a great fit for my first contribution.

[CuteChuanChuan]

Hi @TomasDarquier,

just wanted to follow up and see if you're still actively working on this issue.
If not, I'm happy to contribute and help move it forward.
Thank you.

[TomasDarquier]

Hi @CuteChuanChuan . I initially intended to work on this issue, but I’ve been caught up with other commitments and haven’t had the chance to get to it.

Please feel free to take it on and thanks a lot for picking it up.

[CuteChuanChuan]

Hi @ajantha-bhat ,

I'm working on fixing ErrorProne warnings and have two that I'd like guidance on:

1. DangerousParallelStreamUsage in BigQueryMetastoreClientImpl#491: Link to code
   Context: I noticed that .parallel() is mostly used in test files throughout the codebase. The ParallelIterable class seems not designed for this situation.
   Question: Should I simply remove the .parallel() call to align with the project patterns?
2. ImmutableEnumChecker in Timestamps#47: Link to code
   Context: The SerializableFunction field isn't annotated with Immutable. I noticed that Dates enum uses a static nested class with Immutable annotation.
   Question: Should I follow the same pattern as Dates - creating an Immutable static nested class?

Process Question
Should these two fixes be in separate PRs, or is it fine to include both in a single "Fix ErrorProne warnings" PR?

Thanks for your guidance!

[nastra]

DangerousParallelStreamUsage in BigQueryMetastoreClientImpl#491: Link to code
Context: I noticed that .parallel() is mostly used in test files throughout the codebase. The ParallelIterable class seems not designed for this situation.
Question: Should I simply remove the .parallel() call to align with the project patterns?

@talatuyarer was the .parallel() on the Stream added on purpose in BigQueryMetastoreClientImpl or can we remove it?

[talatuyarer]

@nastra There is some users which they can have huge table list. It was just a little improvement to process their result faster.

[CuteChuanChuan]

@nastra There is some users which they can have huge table list. It was just a little improvement to process their result faster.

Hi @nastra and @ajantha-bhat,
It seems that parallel() was added on purpose; therefore, would adding @SuppressWarnings("DangerousParallelStreamUsage") be an acceptable solution?

Moreover, do you have any guidance or suggestions on adding a static nested class to address the ImmutableEnumChecker warning in Timestamps#47?

Thanks!

[nastra]

On the contrary the implementation of Java parallel streams uses a globally shared ForkJoinPool and does not allow you to provide your own pool. Fork/join pools implement work-stealing, where any thread might steal a task from a different thread's queue when blocked waiting for a subtask to complete. This might not seem like an issue at first glance, but if you use .parallel() for short tasks extensively throughout your codebase and later on you add one piece of code that uses .parallel() for long (e.g. I/O) tasks, the other parts of your codebase that use .parallel(), and that you'd expect to have consistent performance, might experience performance degradation for no apparent reason. The reason is work stealing.
You can suppress this warning if you are certain that all your code will always only use .parallel() for short tasks, but even then, you have no real control over the level of parallelism, so you're still better off using MoreStreams (linked above)
You can find more info here: https://stackoverflow.com/a/54581148/7182570
(see https://github.com/palantir/gradle-baseline#baseline-error-prone-checks)

@talatuyarer can you please double-check whether we should really be using parallel streams there? It would be good to either exclude the warning or remove parallel usage there (I'd prefer to remove it).

@CuteChuanChuan there's also now the below ones that would be good to fix:

> Task :iceberg-flink:iceberg-flink-2.0:compileJava
/Users/eduard.tudenhoefner/Development/workspace/iceberg/flink/v2.0/flink/src/main/java/org/apache/iceberg/flink/sink/dynamic/DynamicWriter.java:202: warning: [MixedMutabilityReturnType] This method returns both mutable and immutable collections or maps from different paths. This may be confusing for users of the method.
  private static List<Integer> getEqualityFields(Table table, List<Integer> equalityFieldIds) {
                               ^
    (see https://errorprone.info/bugpattern/MixedMutabilityReturnType)
  Did you mean 'private static ImmutableList<Integer> getEqualityFields(Table table, List<Integer> equalityFieldIds) {'?
/Users/eduard.tudenhoefner/Development/workspace/iceberg/flink/v2.0/flink/src/main/java/org/apache/iceberg/flink/sink/dynamic/DynamicRecordInternalSerializer.java:234: warning: [ObjectsHashCodePrimitive] Objects.hashCode(Object o) should not be passed a primitive value
    return Objects.hashCode(writeSchemaAndSpec);
                           ^
    (see https://errorprone.info/bugpattern/ObjectsHashCodePrimitive)
  Did you mean 'return Boolean.hashCode(writeSchemaAndSpec);'?

ImmutableEnumChecker in Timestamps#47: Link to code
Context: The SerializableFunction field isn't annotated with Immutable. I noticed that Dates enum uses a static nested class with Immutable annotation.
Question: Should I follow the same pattern as Dates - creating an Immutable static nested class?

@CuteChuanChuan I'm not seeing that one locally. Could you please paste the full errorprone warning msg here?

[ajantha-bhat]

@nastra and @CuteChuanChuan : How about using Tasks.foreach like other parts of the code? I don't see any supression for DangerousParallelStreamUsage in the production code.

Question: Should I follow the same pattern as Dates - creating an Immutable static nested class?

+1 to follow Dates enum, if we don't have breaking change. If not, we can suppress it if it is a breaking change.

Should these two fixes be in separate PRs, or is it fine to include both in a single "Fix ErrorProne warnings" PR?

I think single PR is fine.

[CuteChuanChuan]

@nastra

ImmutableEnumChecker errorprone warning msg as follows:

iceberg/api/src/main/java/org/apache/iceberg/transforms/Timestamps.java:47: warning: [ImmutableEnumChecker] enums should be immutable: 'Timestamps' has field 'apply' of type 'org.apache.iceberg.util.SerializableFunction<java.lang.Long,java.lang.Integer>', the declaration of type 'org.apache.iceberg.util.SerializableFunction<java.lang.Long,java.lang.Integer>' is not annotated with @com.google.errorprone.annotations.Immutable
  private final SerializableFunction<Long, Integer> apply;
                                                    ^
    (see [https://errorprone.info/bugpattern/ImmutableEnumChecker)`](https://errorprone.info/bugpattern/ImmutableEnumChecker)%60)
[ImmutableEnumChecker] enums should be immutable: 'Timestamps' has field 'apply' of type 'org.apache.iceberg.util.SerializableFunction<java.lang.Long,java.lang.Integer>', the declaration of type 'org.apache.iceberg.util.SerializableFunction<java.lang.Long,java.lang.Integer>' is not annotated with @com.google.errorprone.annotations.Immutable

I will address the MixedMutabilityReturnType (introduced in commit b8cc8eb), and the ObjectsHashCodePrimitive is already solved in my draft PR. Thanks for the heads-up!

@ajantha-bhat

Thank you for the guidance on ImmutableEnumChecker, and I will give it a try.
Regarding the DangerousParallelStreamUsage, I will also try using Tasks.foreach to align with the project pattern. Thanks for pointing this out.
After processing all the errorprone warnings, I will submit the PR ASAP.

[CuteChuanChuan]

Hi @ajantha-bhat and @nastra,

I've submitted a PR to address issue #13178 regarding ErrorProne warnings in the build process.
Could you please take a look when you have a chance? I'd appreciate your feedback.
Thank you!

Hi @talatuyarer,

I've replaced the parallel stream with Tasks.foreach() to align with the project's established patterns while preserving your original logic.

Could you please take a look? Thanks!