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 #44055

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

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

@@ -16,8 +16,10 @@
 */
 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

@@ -45,12 +45,14 @@ static ErrorValue fromProtocol(Flight.SetSessionOptionsResult.ErrorValue s) {
      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

    Flight.SetSessionOptionsResult.ErrorValue toProtocol() {
      return Flight.SetSessionOptionsResult.ErrorValue.values()[ordinal()];
    }
  }

  /** 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

@@ -277,7 +278,8 @@ Optional<Map<Object, Object>> getUrlsArgs(String url) throws SQLException {

  static Properties lowerCasePropertyKeys(final Properties properties) {
    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

@@ -71,6 +71,7 @@ public String getString() {
  }

  @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

@@ -241,7 +243,7 @@ public Object get(final Properties properties) {
      Preconditions.checkNotNull(properties, "Properties cannot be null.");
      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

@@ -220,7 +220,10 @@ public void testShouldFailToPrepareStatementForBadStatement() {
       */
      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

@@ -1240,7 +1240,7 @@ public static class PreparedStatement implements AutoCloseable {
     *     used in the {@code PreparedStatement} setters.
     */
    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

@@ -1240,7 +1240,7 @@ public static class PreparedStatement implements AutoCloseable {
     *     used in the {@code PreparedStatement} setters.
     */
    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

@@ -1240,7 +1240,7 @@ public static class PreparedStatement implements AutoCloseable {
     *     used in the {@code PreparedStatement} setters.
     */
    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

@@ -42,7 +42,10 @@ public class Filter {

  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

@@ -45,7 +45,10 @@ public class Projector {

  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

@@ -154,13 +154,13 @@ public static class ConsumeState {

    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

@@ -283,6 +287,7 @@ public Void visit(NullVector vector, Void value) {
  }

  @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

@@ -202,6 +202,7 @@ public Void visit(NullVector vector, Void value) {
  }

  @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

@@ -314,6 +316,7 @@ public Void visit(NullVector vector, Void value) {
  }

  @Override
+  @SuppressWarnings("VoidUsed")


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

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

@@ -176,6 +176,7 @@ public void testShouldGetStringMethodFromDecimalVector(Supplier<ValueVector> vec

  @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

@@ -73,7 +75,8 @@ public boolean equals(Object signature) {

  @Override
  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

    node = n.toProtobuf();
    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

    node = n.toProtobuf();
    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

@@ -385,7 +385,7 @@ public static void concatBits(
    }

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

@@ -138,12 +144,12 @@ public Boolean visit(LargeListViewVector left, Void value) {

  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

@@ -62,10 +62,16 @@ public TypeEqualsVisitor(ValueVector right, boolean checkName, boolean checkMeta
  }

  /** 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

@@ -372,6 +376,16 @@ public boolean equals(VectorSchemaRoot other) {
    return true;
  }

+  @Override
+  public int hashCode() {


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

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

vibhatha mentioned this pull request Sep 12, 2024

[Java] ErrorProne Warnings to be considered as Errors #43941

Open

12 tasks

vibhatha added 2 commits September 13, 2024 07:21

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

Conversation

vibhatha commented Sep 11, 2024 • edited by github-actions bot Loading

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danepitkin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vibhatha commented Sep 11, 2024 •

edited by github-actions bot

Loading