GH-44055: [Java] Finalize ErrorProne Warnings to be considered as Errors #44056

vibhatha · 2024-09-11T02:39:54Z

Rationale for this change

A series of PRs have been created to mark warnings as errors in a module-based approach. As this step has been completed, moving the config back to the parent pom would be the best to keep things clean and easy to maintain.

What changes are included in this PR?

Removing the pom changes in each module and updating the parent pom with the required configurations.

Are these changes tested?

Tested from existing test cases.

Are there any user-facing changes?

N/A

GitHub Issue: [Java] Finalize ErrorProne Warnings to be considered as Errors arrow-java#53
GitHub Issue: ARROW-49: [Python] Add Column and Table wrapper interface #53

lidavidm · 2024-09-13T08:31:52Z

java/flight/flight-core/src/main/java/org/apache/arrow/flight/ServerSessionMiddleware.java

 */
 package org.apache.arrow.flight;

+import com.google.common.base.Splitter;


Ideally we wouldn't use Guava? I suppose as long as this interface seems stable...

Is there an apache-commons equivalent we can use?

I will take a look, this was actually suggested by the ErrorProne library, it wasn't my first choice.

lidavidm · 2024-09-13T08:35:09Z

java/flight/flight-core/src/main/java/org/apache/arrow/flight/SetSessionOptionsResult.java

      return values()[s.getNumber()];
    }

+    @SuppressWarnings("EnumOrdinal")


IMO, the right way to do this would be to add a constructor and store the Protobuf enum value in the enum instance

Right, that is the ideal way. Let me track down and change the similar other instances.

lidavidm · 2024-09-13T08:35:32Z

java/flight/flight-core/src/main/java/org/apache/arrow/flight/SetSessionOptionsResult.java

  }

  /** Per-option extensible error response container. */
+  @SuppressWarnings("JavaLangClash")


What was the problem here?

It's clashing with java.lang.Error https://docs.oracle.com/javase/8/docs/api/java/lang/Error.html

+1 for renaming it, if it's not an external API change in some way

This is public, it will be a breaking change.

Right, the issue is as @danepitkin mentioned. Shall we leave the warning suppressed?

Yes, let's leave the suppression in place.

lidavidm · 2024-09-13T08:36:00Z

java/flight/flight-core/src/main/java/org/apache/arrow/flight/grpc/AddWritableBuffer.java

+      throw new RuntimeException(
+          "Failed to initialize AddWritableBuffer, falling back to slow path", ex);


I think this intentionally did not throw, as evidenced by the "falling back" message

lidavidm · 2024-09-13T08:36:07Z

java/flight/flight-core/src/main/java/org/apache/arrow/flight/grpc/GetReadableBuffer.java

+      throw new RuntimeException(
+          "Failed to initialize GetReadableBuffer, falling back to slow path", e);


lidavidm · 2024-09-13T08:36:54Z

...t/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/ArrowFlightJdbcDriver.java

    final Properties resultProperty = new Properties();
-    properties.forEach((k, v) -> resultProperty.put(k.toString().toLowerCase(), v));
+    properties.forEach(
+        (k, v) -> resultProperty.put(k.toString().toLowerCase(Locale.getDefault()), v));


let's use Locale.ROOT rather than the system locale

lidavidm · 2024-09-13T08:39:44Z

...org/apache/arrow/driver/jdbc/accessor/impl/numeric/ArrowFlightJdbcDecimalVectorAccessor.java

  }

  @Override
+  @SuppressWarnings("BigDecimalEquals")


Use compareTo instead https://stackoverflow.com/questions/10950914/how-to-check-if-bigdecimal-variable-0-in-java

lidavidm · 2024-09-13T08:39:54Z

...c-core/src/main/java/org/apache/arrow/driver/jdbc/utils/ArrowFlightConnectionConfigImpl.java

      Object value = properties.get(camelName);
      if (value == null) {
-        value = properties.get(camelName.toLowerCase());
+        value = properties.get(camelName.toLowerCase(Locale.getDefault()));


Locale.ROOT

lidavidm · 2024-09-13T08:40:05Z

...c-core/src/test/java/org/apache/arrow/driver/jdbc/ArrowFlightStatementExecuteUpdateTest.java

      assertThat(
          e.getMessage(),
-          is(format("Error while executing SQL \"%s\": Query not found", badQuery)));
+          is(


Would prefer contains

lidavidm · 2024-09-13T08:40:46Z

java/flight/flight-sql/src/main/java/org/apache/arrow/flight/sql/FlightSqlClient.java

     */
    public void setParameters(final VectorSchemaRoot parameterBindingRoot) {
-      if (parameterBindingRoot == this.parameterBindingRoot) {
+      if (parameterBindingRoot.equals(this.parameterBindingRoot)) {


I think this was intentionally an identity check

Right. I will leave it as it is, and add the suppressing.

I would recommend adding a comment alongside warning suppressions for future readers. It's not always obvious if a Warning suppression is the result of a temporary bug, laziness from the developers (not you, just in general :P), or is truly an unhelpful warning.

For example, we would never want to un-suppress this warning because we want to compare object references, but for the Flatbuffers module name warning in an older PR, we want to eventually remove it once fixed.

danepitkin

Heroic effort 👏 Nice job @vibhatha.

danepitkin · 2024-09-13T15:16:26Z

java/flight/flight-sql/src/main/java/org/apache/arrow/flight/sql/FlightSqlClient.java

     */
    public void setParameters(final VectorSchemaRoot parameterBindingRoot) {
-      if (parameterBindingRoot == this.parameterBindingRoot) {
+      if (parameterBindingRoot.equals(this.parameterBindingRoot)) {


I would recommend adding a comment alongside warning suppressions for future readers. It's not always obvious if a Warning suppression is the result of a temporary bug, laziness from the developers (not you, just in general :P), or is truly an unhelpful warning.

danepitkin · 2024-09-13T15:18:35Z

java/flight/flight-sql/src/main/java/org/apache/arrow/flight/sql/FlightSqlClient.java

     */
    public void setParameters(final VectorSchemaRoot parameterBindingRoot) {
-      if (parameterBindingRoot == this.parameterBindingRoot) {
+      if (parameterBindingRoot.equals(this.parameterBindingRoot)) {


For example, we would never want to un-suppress this warning because we want to compare object references, but for the Flatbuffers module name warning in an older PR, we want to eventually remove it once fixed.

danepitkin · 2024-09-13T15:28:19Z

java/gandiva/src/main/java/org/apache/arrow/gandiva/evaluator/Filter.java

  private final JniWrapper wrapper;
  private final long moduleId;
+
+  @SuppressWarnings("UnusedVariable")


This actually looks unused. It's private, and not used after object instantiation. Should we remove it?

danepitkin · 2024-09-13T15:32:16Z

java/gandiva/src/main/java/org/apache/arrow/gandiva/evaluator/Projector.java

  private JniWrapper wrapper;
  private final long moduleId;
+
+  @SuppressWarnings("UnusedVariable")


We can probably delete this, too.

danepitkin · 2024-09-13T15:43:29Z

java/performance/src/main/java/org/apache/arrow/adapter/jdbc/JdbcAdapterBenchmarks.java

    private JdbcConsumer<BitVector> bitConsumer;

-    private JdbcToArrowConfig config;
+    // private JdbcToArrowConfig config;


Should this be deleted?

danepitkin · 2024-09-13T15:49:31Z

java/memory/pom.xml

+        <groupId>org.apache.maven.plugins</groupId>
+        <artifactId>maven-compiler-plugin</artifactId>
+        <configuration>
+          <compilerArgs combine.children="override">


Are we trying to override and disable warnings-as-errors for this module? If so, it should be combine.self="override".

danepitkin · 2024-09-13T18:09:33Z

java/vector/src/main/java/org/apache/arrow/vector/validate/ValidateVectorBufferVisitor.java

  }

  @Override
+  @SuppressWarnings("VoidUsed")


You could use vector.getUnderlyingVector().accept(this, null); and get rid of the suppressed warning.

danepitkin · 2024-09-13T18:10:02Z

java/vector/src/main/java/org/apache/arrow/vector/validate/ValidateVectorDataVisitor.java

  }

  @Override
+  @SuppressWarnings("VoidUsed")


same here. vector.getUnderlyingVector().accept(this, null);

danepitkin · 2024-09-13T18:10:17Z

java/vector/src/main/java/org/apache/arrow/vector/validate/ValidateVectorVisitor.java

  }

  @Override
+  @SuppressWarnings("VoidUsed")


lidavidm · 2024-09-14T00:58:06Z

...apache/arrow/driver/jdbc/accessor/impl/numeric/ArrowFlightJdbcDecimalVectorAccessorTest.java


  @ParameterizedTest
  @MethodSource("data")
+  @SuppressWarnings("BigDecimalEquals")


use compareTo

lidavidm · 2024-09-14T00:58:38Z

java/gandiva/src/main/java/org/apache/arrow/gandiva/evaluator/FunctionSignature.java

  public int hashCode() {
-    return Objects.hashCode(this.name.toLowerCase(), this.returnType, this.paramTypes);
+    return Objects.hashCode(
+        this.name.toLowerCase(Locale.getDefault()), this.returnType, this.paramTypes);


Locale.ROOT

lidavidm · 2024-09-14T00:59:42Z

java/gandiva/src/main/java/org/apache/arrow/gandiva/exceptions/GandivaException.java

+  }
+
+  @Override
+  public String getMessage() {


Why is the trivial override needed?

lidavidm · 2024-09-14T01:35:19Z

java/gandiva/src/test/java/org/apache/arrow/gandiva/expression/TreeBuilderTest.java

    assertEquals(10, node.getIntNode().getValue());

-    n = TreeBuilder.makeLiteral(new Long(50));
+    n = TreeBuilder.makeLiteral(Long.valueOf(50));


50L doesn't work?

lidavidm · 2024-09-14T01:35:23Z

java/gandiva/src/test/java/org/apache/arrow/gandiva/expression/TreeBuilderTest.java

    assertEquals(50, node.getLongNode().getValue());

-    Float f = new Float(2.5);
+    Float f = (float) 2.5;


And does it need to be boxed?

lidavidm · 2024-09-14T01:40:26Z

java/vector/src/main/java/org/apache/arrow/vector/BitVectorHelper.java


    // copy the first bit set
-    if (input1 != output) {
+    if (!input1.equals(output)) {


Hmm, I wonder if this was intentional or not.

lidavidm · 2024-09-14T01:49:20Z

java/vector/src/main/java/org/apache/arrow/vector/compare/TypeEqualsVisitor.java

  private boolean compareField(Field leftField, Field rightField) {

-    if (leftField == rightField) {
+    if (leftField.equals(rightField)) {


I think this was intentional: it's a cheap "is this actually the same object" check, before doing the rest of the checks below

lidavidm · 2024-09-14T01:49:42Z

java/vector/src/main/java/org/apache/arrow/vector/compare/TypeEqualsVisitor.java


  /** Check type equals without passing IN param in VectorVisitor. */
+  @SuppressWarnings("NonOverridingEquals")
  public boolean equals(ValueVector left) {


...this shoulda been named something different but it's way too late now

lidavidm · 2024-09-14T01:51:08Z

java/vector/src/main/java/org/apache/arrow/vector/VectorSchemaRoot.java

  }

+  @Override
+  public int hashCode() {


This is a mutable object, I'm not sure we want it to have a hashCode...

github-actions · 2024-11-26T19:13:21Z

⚠️ GitHub issue #44055 has no components, please add labels for components.

vibhatha added 2 commits September 11, 2024 08:02

fix: cleaning up adapter module

0472277

fix: removing plugin

369ad2d

vibhatha changed the title ~~[Java] Finalize ErrorProne Warnings to be considered as Errors #44055~~ GH-44055: [Java] Finalize ErrorProne Warnings to be considered as Errors Sep 11, 2024

github-actions bot added the awaiting review Awaiting review label Sep 11, 2024

fix: removing configs from dataset module

2f91c9b

github-actions bot added the Component: Java label Sep 11, 2024

vibhatha added 7 commits September 11, 2024 10:21

fix: cleaning vector module (unexpected)

0e79ee9

fix: cleaning vector module v2

de95e9c

fix: cleaning vector module v3

a99a701

fix: cleaning vector module v4

f8bc623

fix: cleaning vector module v5

ab93ca8

fix: cleaning vector module v6

7c1d829

fix: cleaning vector module v7

003e1ab

vibhatha marked this pull request as ready for review September 13, 2024 08:29

vibhatha requested a review from lidavidm as a code owner September 13, 2024 08:29

lidavidm reviewed Sep 13, 2024

View reviewed changes

github-actions bot added awaiting changes Awaiting changes and removed awaiting review Awaiting review labels Sep 13, 2024

danepitkin reviewed Sep 13, 2024

View reviewed changes

lidavidm reviewed Sep 14, 2024

View reviewed changes

fix: adding splitter

ceaf567

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Sep 15, 2024

vibhatha added 2 commits September 15, 2024 14:19

fix: adding lang3 for jdbc

5b85701

fix:reverting a change temporarily

1354ad0

		throw new RuntimeException(
		"Failed to initialize AddWritableBuffer, falling back to slow path", ex);

		throw new RuntimeException(
		"Failed to initialize GetReadableBuffer, falling back to slow path", e);

GH-44055: [Java] Finalize ErrorProne Warnings to be considered as Errors #44056

Are you sure you want to change the base?

GH-44055: [Java] Finalize ErrorProne Warnings to be considered as Errors #44056

Uh oh!

Conversation

vibhatha commented Sep 11, 2024 • edited by github-actions bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danepitkin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

vibhatha commented Sep 11, 2024 •

edited by github-actions bot

Loading