You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2022/04/20 22:19:27 UTC

[GitHub] [arrow] davisusanibar opened a new pull request, #12941: ARROW-15755: [Java] WIP - Support Java 17

davisusanibar opened a new pull request, #12941:
URL: https://github.com/apache/arrow/pull/12941

   Changes needed to support JDK17:
   
   - There are some [errorprone](https://errorprone.info/docs/installation) suppress warnings needed to be able to continue using JSE17 with JSE1.8 source/target. This need to be migrated to new JSE X base on [ML discussion](https://lists.apache.org/thread/phpgpydtt3yrgnncdyv4qdq1gf02s0yj).
   - JSE11+ is modular for that reason was needed to add `--add-exports` and `--add-opens`
   - The --add-exports option allows code in the target module to access types in the named package of the source module if the target module reads the source module.
   - If you have to allow code on the class path to do deep reflection to access nonpublic members, then use the --add-opens runtime option.
   - To be able to running unit test was needed to configure `maven-surefire-plugin` with `<argLine>--add-opens=java.base/java.nio=ALL-UNNAMED</argLine>`
   
   


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

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

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

[GitHub] [arrow] github-actions[bot] commented on pull request #12941: ARROW-15755: [Java] WIP - Support Java 17

Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #12941:
URL: https://github.com/apache/arrow/pull/12941#issuecomment-1104514508

   https://issues.apache.org/jira/browse/ARROW-15755


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

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

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

[GitHub] [arrow] lidavidm commented on pull request #12941: ARROW-15755: [Java] Support Java 17

Posted by GitBox <gi...@apache.org>.
lidavidm commented on PR #12941:
URL: https://github.com/apache/arrow/pull/12941#issuecomment-1105273044

   Either that, or we should make sure to validate JDK17 during the 8.0.0 vote.


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

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

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

[GitHub] [arrow] davisusanibar commented on a diff in pull request #12941: ARROW-15755: [Java] Support Java 17

Posted by GitBox <gi...@apache.org>.
davisusanibar commented on code in PR #12941:
URL: https://github.com/apache/arrow/pull/12941#discussion_r861204367


##########
java/flight/flight-core/src/test/java/org/apache/arrow/flight/TestClientMiddleware.java:
##########
@@ -246,12 +246,13 @@ public FlightClientMiddleware onCallStarted(CallInfo info) {
   }
 
   // Used to test that middleware can send and receive multi-valued text and binary headers.
-  static final Map<String, List<byte[]>> EXPECTED_BINARY_HEADERS = new HashMap<String, List<byte[]>>() {{
-      put("x-binary-bin", Arrays.asList(new byte[] {0}, new byte[]{1}));
-    }};
-  static final Map<String, List<String>> EXPECTED_TEXT_HEADERS = new HashMap<String, List<String>>() {{
-      put("x-text", Arrays.asList("foo", "bar"));
-    }};
+  static final Map<String, List<byte[]>> EXPECTED_BINARY_HEADERS = new HashMap<String, List<byte[]>>();
+  static final Map<String, List<String>> EXPECTED_TEXT_HEADERS = new HashMap<String, List<String>>();
+
+  {

Review Comment:
   Deleted



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

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

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

[GitHub] [arrow] davisusanibar commented on a diff in pull request #12941: ARROW-15755: [Java] Support Java 17

Posted by GitBox <gi...@apache.org>.
davisusanibar commented on code in PR #12941:
URL: https://github.com/apache/arrow/pull/12941#discussion_r855533265


##########
java/flight/flight-sql/src/main/java/org/apache/arrow/flight/sql/FlightSqlClient.java:
##########
@@ -77,6 +77,7 @@
 /**
  * Flight client with Flight SQL semantics.
  */
+@SuppressWarnings({"ProtoBuilderReturnValueIgnored", "ReturnValueIgnored"})

Review Comment:
   Changed



##########
java/memory/memory-core/src/test/java/org/apache/arrow/memory/util/TestArrowBufPointer.java:
##########
@@ -111,6 +111,7 @@ public void testNullPointersHashCode() {
   }
 
   @Test
+  @SuppressWarnings("ReturnValueIgnored")

Review Comment:
   Changed



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

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

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

[GitHub] [arrow] davisusanibar commented on a diff in pull request #12941: ARROW-15755: [Java] Support Java 17

Posted by GitBox <gi...@apache.org>.
davisusanibar commented on code in PR #12941:
URL: https://github.com/apache/arrow/pull/12941#discussion_r855533449


##########
java/pom.xml:
##########
@@ -736,6 +738,7 @@
            See https://github.com/jbosstools/m2e-apt/issues/62 for details
       -->
       <activation>
+        <jdk>1.8</jdk>

Review Comment:
   Changed



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

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

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

[GitHub] [arrow] davisusanibar commented on pull request #12941: ARROW-15755: [Java] Support Java 17

Posted by GitBox <gi...@apache.org>.
davisusanibar commented on PR #12941:
URL: https://github.com/apache/arrow/pull/12941#issuecomment-1113676772

   > Good point. @davisusanibar can we include the docs changes here?
   
   Thanks @toddfarmer. Just updated.


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

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

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

[GitHub] [arrow] davisusanibar commented on a diff in pull request #12941: ARROW-15755: [Java] Support Java 17

Posted by GitBox <gi...@apache.org>.
davisusanibar commented on code in PR #12941:
URL: https://github.com/apache/arrow/pull/12941#discussion_r861215328


##########
java/flight/flight-core/src/test/java/org/apache/arrow/flight/TestClientMiddleware.java:
##########
@@ -246,12 +246,13 @@ public FlightClientMiddleware onCallStarted(CallInfo info) {
   }
 
   // Used to test that middleware can send and receive multi-valued text and binary headers.
-  static final Map<String, List<byte[]>> EXPECTED_BINARY_HEADERS = new HashMap<String, List<byte[]>>() {{
-      put("x-binary-bin", Arrays.asList(new byte[] {0}, new byte[]{1}));
-    }};
-  static final Map<String, List<String>> EXPECTED_TEXT_HEADERS = new HashMap<String, List<String>>() {{
-      put("x-text", Arrays.asList("foo", "bar"));
-    }};
+  static final Map<String, List<byte[]>> EXPECTED_BINARY_HEADERS = new HashMap<String, List<byte[]>>();
+  static final Map<String, List<String>> EXPECTED_TEXT_HEADERS = new HashMap<String, List<String>>();
+
+  {

Review Comment:
   My mistake, just applied suggestion



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

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

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

[GitHub] [arrow] lidavidm commented on a diff in pull request #12941: ARROW-15755: [Java] Support Java 17

Posted by GitBox <gi...@apache.org>.
lidavidm commented on code in PR #12941:
URL: https://github.com/apache/arrow/pull/12941#discussion_r862131278


##########
docs/source/developers/java/building.rst:
##########
@@ -32,7 +32,7 @@ Arrow Java uses the `Maven <https://maven.apache.org/>`_ build system.
 
 Building requires:
 
-* JDK 8, 9, 10, or 11, but only JDK 11 is tested in CI
+* JDK 8, 9, 10, 11, 17, and 18, but only JDK 11 is tested in CI.

Review Comment:
   ```suggestion
   * JDK 8, 9, 10, 11, 17, or 18, but only JDK 11 is tested in CI.
   ```



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

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

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

[GitHub] [arrow] davisusanibar commented on pull request #12941: ARROW-15755: [Java] Support Java 17

Posted by GitBox <gi...@apache.org>.
davisusanibar commented on PR #12941:
URL: https://github.com/apache/arrow/pull/12941#issuecomment-1112270441

   > Hi @davisusanibar I have created this draft PR to add Java 17 to CI (#13021). As you can see from the triggered jobs, there is a new Java job: ![image](https://user-images.githubusercontent.com/639755/165702289-844ec524-3541-4613-8751-833c47e4912a.png) It is obviously failing (https://github.com/apache/arrow/runs/6208174355?check_suite_focus=true) because the fixes for Java 17 are on your PR but I have cherry-picked the commit locally on top of your branch and I have validated the build is working and tests are passing. Tested locally with:
   > 
   > ```
   > $ JDK=17 MAVEN=3.8.5 JDK_NAME=openjdk docker-compose run --rm debian-java
   > ```
   > 
   > I have validated java version was the correct when testing:
   > 
   > ```
   > bash-4.4# java --version
   > openjdk 17.0.2 2022-01-18
   > OpenJDK Runtime Environment (build 17.0.2+8-86)
   > OpenJDK 64-Bit Server VM (build 17.0.2+8-86, mixed mode, sharing)
   > bash-4.4# exit
   > ```
   > 
   > I am not sure how we do this but probably you might want to cherrypick my commit and apply it on your PR.
   
   Thanks @raulcd. Let me solve pending comments. Consider: in my local test was needed to add this environment variable `"--add-opens=java.base/java.nio=ALL-UNNAMED"`, but as suggested let merge this and test your changes them. 


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

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

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

[GitHub] [arrow] lidavidm commented on pull request #12941: ARROW-15755: [Java] Support Java 17

Posted by GitBox <gi...@apache.org>.
lidavidm commented on PR #12941:
URL: https://github.com/apache/arrow/pull/12941#issuecomment-1113628654

   @github-actions crossbow submit *java*


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

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

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

[GitHub] [arrow] lidavidm commented on a diff in pull request #12941: ARROW-15755: [Java] Support Java 17

Posted by GitBox <gi...@apache.org>.
lidavidm commented on code in PR #12941:
URL: https://github.com/apache/arrow/pull/12941#discussion_r855524920


##########
java/flight/flight-sql/src/main/java/org/apache/arrow/flight/sql/FlightSqlClient.java:
##########
@@ -346,7 +345,7 @@ public FlightInfo getExportedKeys(final TableRef tableRef, final CallOption... o
     }
 
     Objects.requireNonNull(tableRef.getTable());
-    builder.setTable(tableRef.getTable()).build();

Review Comment:
   The warning is only saying that you're ignoring the reuslt of `build()`. So you can presumably remove the `build`, but keep the rest of the line.



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

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

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

[GitHub] [arrow] lidavidm commented on a diff in pull request #12941: ARROW-15755: [Java] Support Java 17

Posted by GitBox <gi...@apache.org>.
lidavidm commented on code in PR #12941:
URL: https://github.com/apache/arrow/pull/12941#discussion_r854681807


##########
java/pom.xml:
##########
@@ -763,6 +766,60 @@
       </build>
     </profile>
 
+    <profile>
+      <id>error-prone-jdk11+</id>
+      <activation>
+        <jdk>[11,)</jdk>
+        <property>
+          <name>!m2e.version</name>
+        </property>
+      </activation>
+      <build>
+        <plugins>
+          <plugin>
+            <groupId>org.apache.maven.plugins</groupId>
+            <artifactId>maven-compiler-plugin</artifactId>
+            <configuration>
+              <source>8</source>
+              <target>8</target>
+              <encoding>UTF-8</encoding>
+              <compilerArgs combine.children="append">
+                <arg>-XDcompilePolicy=simple</arg>
+                <arg>
+                  -Xplugin:ErrorProne \
+                  -XepExcludedPaths:.*/(target/generated-sources)/.*

Review Comment:
   nit, but can these be two separate `<arg>` blocks or does it not work that way?



##########
java/memory/memory-core/src/test/java/org/apache/arrow/memory/util/TestArrowBufPointer.java:
##########
@@ -111,6 +111,7 @@ public void testNullPointersHashCode() {
   }
 
   @Test
+  @SuppressWarnings("ReturnValueIgnored")

Review Comment:
   Maybe instead of suppresing we can check that the hash code changes/doesn't change?



##########
java/flight/flight-sql/src/main/java/org/apache/arrow/flight/sql/FlightSqlClient.java:
##########
@@ -77,6 +77,7 @@
 /**
  * Flight client with Flight SQL semantics.
  */
+@SuppressWarnings({"ProtoBuilderReturnValueIgnored", "ReturnValueIgnored"})

Review Comment:
   Hmm, this is a very wide scope to suppress. Is it possible to refactor things?



##########
java/flight/flight-core/src/test/java/org/apache/arrow/flight/TestCallOptions.java:
##########
@@ -97,6 +97,7 @@ public void mixedProperties() {
     testHeaders(headers);
   }
 
+  @SuppressWarnings("ReturnValueIgnored")

Review Comment:
   Same here, maybe an `assertTrue(....hasNext())`?



##########
java/pom.xml:
##########
@@ -736,6 +738,7 @@
            See https://github.com/jbosstools/m2e-apt/issues/62 for details
       -->
       <activation>
+        <jdk>1.8</jdk>

Review Comment:
   Hmm, we already have a 1.8-specific profile below. What's the difference?



##########
java/adapter/orc/pom.xml:
##########
@@ -52,10 +52,27 @@
                 </exclusion>
             </exclusions>
         </dependency>
+        <dependency>
+            <groupId>org.apache.hadoop</groupId>
+            <artifactId>hadoop-client-runtime</artifactId>
+            <version>3.3.2</version>
+            <scope>test</scope>
+            <exclusions>
+                <exclusion>
+                    <groupId>commons-logging</groupId>
+                    <artifactId>commons-logging</artifactId>

Review Comment:
   Can we add a brief comment about why we need to exclude these? 



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

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

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

[GitHub] [arrow] davisusanibar commented on a diff in pull request #12941: ARROW-15755: [Java] Support Java 17

Posted by GitBox <gi...@apache.org>.
davisusanibar commented on code in PR #12941:
URL: https://github.com/apache/arrow/pull/12941#discussion_r854689552


##########
java/adapter/orc/pom.xml:
##########
@@ -52,10 +52,27 @@
                 </exclusion>
             </exclusions>
         </dependency>
+        <dependency>
+            <groupId>org.apache.hadoop</groupId>
+            <artifactId>hadoop-client-runtime</artifactId>
+            <version>3.3.2</version>
+            <scope>test</scope>
+            <exclusions>
+                <exclusion>
+                    <groupId>commons-logging</groupId>
+                    <artifactId>commons-logging</artifactId>

Review Comment:
   Yes, the reason is `[WARNING] Rule 0: org.apache.maven.plugins.enforcer.BannedDependencies failed with message:
   Found Banned Dependency: commons-logging:commons-logging:jar:1.1.3`. Let me add some message on the exclude tag.



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

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

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

[GitHub] [arrow] pitrou commented on pull request #12941: ARROW-15755: [Java] Support Java 17

Posted by GitBox <gi...@apache.org>.
pitrou commented on PR #12941:
URL: https://github.com/apache/arrow/pull/12941#issuecomment-1104924412

   @lwhite1 You might want to take a look at this.


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

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

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

[GitHub] [arrow] hashhar commented on a diff in pull request #12941: ARROW-15755: [Java] Support Java 17

Posted by GitBox <gi...@apache.org>.
hashhar commented on code in PR #12941:
URL: https://github.com/apache/arrow/pull/12941#discussion_r856232426


##########
java/pom.xml:
##########
@@ -777,10 +781,40 @@
             <groupId>org.apache.maven.plugins</groupId>
             <artifactId>maven-compiler-plugin</artifactId>
             <configuration>
-              <fork>true</fork>
+              <source>8</source>
+              <target>8</target>
+              <encoding>UTF-8</encoding>
               <compilerArgs combine.children="append">
-                <arg>-J-Xbootclasspath/p:${settings.localRepository}/com/google/errorprone/javac/${errorprone.javac.version}/javac-${errorprone.javac.version}.jar</arg>
+                <arg>-XDcompilePolicy=simple</arg>
+                <arg>
+                  -Xplugin:ErrorProne \
+                  -XepExcludedPaths:.*/(target/generated-sources)/.*
+                </arg>
+                <arg>-J--add-exports=jdk.compiler/com.sun.tools.javac.api=ALL-UNNAMED</arg>
+                <arg>-J--add-exports=jdk.compiler/com.sun.tools.javac.file=ALL-UNNAMED</arg>
+                <arg>-J--add-exports=jdk.compiler/com.sun.tools.javac.main=ALL-UNNAMED</arg>
+                <arg>-J--add-exports=jdk.compiler/com.sun.tools.javac.model=ALL-UNNAMED</arg>
+                <arg>-J--add-exports=jdk.compiler/com.sun.tools.javac.parser=ALL-UNNAMED</arg>
+                <arg>-J--add-exports=jdk.compiler/com.sun.tools.javac.processing=ALL-UNNAMED</arg>
+                <arg>-J--add-exports=jdk.compiler/com.sun.tools.javac.tree=ALL-UNNAMED</arg>
+                <arg>-J--add-exports=jdk.compiler/com.sun.tools.javac.util=ALL-UNNAMED</arg>
+                <arg>-J--add-opens=jdk.compiler/com.sun.tools.javac.code=ALL-UNNAMED</arg>
+                <arg>-J--add-opens=jdk.compiler/com.sun.tools.javac.comp=ALL-UNNAMED</arg>

Review Comment:
   Does this change mean that consumers of this library will still need to pass `add-opens` to get Arrow to work with JDK 17? (I think so since you are just making the compilation work with these arguments, no code is changed to not use those modules?).



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

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

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

[GitHub] [arrow] davisusanibar commented on a diff in pull request #12941: ARROW-15755: [Java] Support Java 17

Posted by GitBox <gi...@apache.org>.
davisusanibar commented on code in PR #12941:
URL: https://github.com/apache/arrow/pull/12941#discussion_r854691427


##########
java/adapter/orc/pom.xml:
##########
@@ -52,10 +52,27 @@
                 </exclusion>
             </exclusions>
         </dependency>
+        <dependency>
+            <groupId>org.apache.hadoop</groupId>
+            <artifactId>hadoop-client-runtime</artifactId>
+            <version>3.3.2</version>
+            <scope>test</scope>
+            <exclusions>
+                <exclusion>
+                    <groupId>commons-logging</groupId>
+                    <artifactId>commons-logging</artifactId>

Review Comment:
   Unit test is working without problems in JSE 8, JSE 11 but not in JSE 17, that is the reason because we added now.



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

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

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

[GitHub] [arrow] lwhite1 commented on a diff in pull request #12941: ARROW-15755: [Java] Support Java 17

Posted by GitBox <gi...@apache.org>.
lwhite1 commented on code in PR #12941:
URL: https://github.com/apache/arrow/pull/12941#discussion_r855235698


##########
java/flight/flight-core/src/test/java/org/apache/arrow/flight/TestClientMiddleware.java:
##########
@@ -246,9 +246,11 @@ public FlightClientMiddleware onCallStarted(CallInfo info) {
   }
 
   // Used to test that middleware can send and receive multi-valued text and binary headers.
+  @SuppressWarnings({"DoubleBraceInitialization"})

Review Comment:
   Maybe just initialize these values (here and below) the old-fashioned way, rather than add the SuppressWarnings?



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

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

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

[GitHub] [arrow] lidavidm commented on a diff in pull request #12941: ARROW-15755: [Java] Support Java 17

Posted by GitBox <gi...@apache.org>.
lidavidm commented on code in PR #12941:
URL: https://github.com/apache/arrow/pull/12941#discussion_r856290962


##########
java/pom.xml:
##########
@@ -777,10 +781,40 @@
             <groupId>org.apache.maven.plugins</groupId>
             <artifactId>maven-compiler-plugin</artifactId>
             <configuration>
-              <fork>true</fork>
+              <source>8</source>
+              <target>8</target>
+              <encoding>UTF-8</encoding>
               <compilerArgs combine.children="append">
-                <arg>-J-Xbootclasspath/p:${settings.localRepository}/com/google/errorprone/javac/${errorprone.javac.version}/javac-${errorprone.javac.version}.jar</arg>
+                <arg>-XDcompilePolicy=simple</arg>
+                <arg>
+                  -Xplugin:ErrorProne \
+                  -XepExcludedPaths:.*/(target/generated-sources)/.*
+                </arg>
+                <arg>-J--add-exports=jdk.compiler/com.sun.tools.javac.api=ALL-UNNAMED</arg>
+                <arg>-J--add-exports=jdk.compiler/com.sun.tools.javac.file=ALL-UNNAMED</arg>
+                <arg>-J--add-exports=jdk.compiler/com.sun.tools.javac.main=ALL-UNNAMED</arg>
+                <arg>-J--add-exports=jdk.compiler/com.sun.tools.javac.model=ALL-UNNAMED</arg>
+                <arg>-J--add-exports=jdk.compiler/com.sun.tools.javac.parser=ALL-UNNAMED</arg>
+                <arg>-J--add-exports=jdk.compiler/com.sun.tools.javac.processing=ALL-UNNAMED</arg>
+                <arg>-J--add-exports=jdk.compiler/com.sun.tools.javac.tree=ALL-UNNAMED</arg>
+                <arg>-J--add-exports=jdk.compiler/com.sun.tools.javac.util=ALL-UNNAMED</arg>
+                <arg>-J--add-opens=jdk.compiler/com.sun.tools.javac.code=ALL-UNNAMED</arg>
+                <arg>-J--add-opens=jdk.compiler/com.sun.tools.javac.comp=ALL-UNNAMED</arg>

Review Comment:
   Can we add point (2) to the docs?



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

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

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

[GitHub] [arrow] github-actions[bot] commented on pull request #12941: ARROW-15755: [Java] Support Java 17

Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #12941:
URL: https://github.com/apache/arrow/pull/12941#issuecomment-1113629728

   Revision: 439403bdfb15f578dfe9dc29fc8d285dbc719c10
   
   Submitted crossbow builds: [ursacomputing/crossbow @ actions-1988](https://github.com/ursacomputing/crossbow/branches/all?query=actions-1988)
   
   |Task|Status|
   |----|------|
   |java-jars|[![Github Actions](https://github.com/ursacomputing/crossbow/workflows/Crossbow/badge.svg?branch=actions-1988-github-java-jars)](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-1988-github-java-jars)|
   |verify-rc-source-java-linux-almalinux-8-amd64|[![Github Actions](https://github.com/ursacomputing/crossbow/workflows/Crossbow/badge.svg?branch=actions-1988-github-verify-rc-source-java-linux-almalinux-8-amd64)](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-1988-github-verify-rc-source-java-linux-almalinux-8-amd64)|
   |verify-rc-source-java-linux-conda-latest-amd64|[![Github Actions](https://github.com/ursacomputing/crossbow/workflows/Crossbow/badge.svg?branch=actions-1988-github-verify-rc-source-java-linux-conda-latest-amd64)](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-1988-github-verify-rc-source-java-linux-conda-latest-amd64)|
   |verify-rc-source-java-linux-ubuntu-18.04-amd64|[![Github Actions](https://github.com/ursacomputing/crossbow/workflows/Crossbow/badge.svg?branch=actions-1988-github-verify-rc-source-java-linux-ubuntu-18.04-amd64)](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-1988-github-verify-rc-source-java-linux-ubuntu-18.04-amd64)|
   |verify-rc-source-java-linux-ubuntu-20.04-amd64|[![Github Actions](https://github.com/ursacomputing/crossbow/workflows/Crossbow/badge.svg?branch=actions-1988-github-verify-rc-source-java-linux-ubuntu-20.04-amd64)](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-1988-github-verify-rc-source-java-linux-ubuntu-20.04-amd64)|
   |verify-rc-source-java-linux-ubuntu-22.04-amd64|[![Github Actions](https://github.com/ursacomputing/crossbow/workflows/Crossbow/badge.svg?branch=actions-1988-github-verify-rc-source-java-linux-ubuntu-22.04-amd64)](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-1988-github-verify-rc-source-java-linux-ubuntu-22.04-amd64)|
   |verify-rc-source-java-macos-amd64|[![Github Actions](https://github.com/ursacomputing/crossbow/workflows/Crossbow/badge.svg?branch=actions-1988-github-verify-rc-source-java-macos-amd64)](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-1988-github-verify-rc-source-java-macos-amd64)|


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

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

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

[GitHub] [arrow] ursabot commented on pull request #12941: ARROW-15755: [Java] Support Java 17

Posted by GitBox <gi...@apache.org>.
ursabot commented on PR #12941:
URL: https://github.com/apache/arrow/pull/12941#issuecomment-1119381437

   Benchmark runs are scheduled for baseline = 3f2c33f59160a734cc42137d6e1f12e32cf3ec92 and contender = 760ad20067418e0c9a4d6bf524c52f50783bc135. 760ad20067418e0c9a4d6bf524c52f50783bc135 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
   Conbench compare runs links:
   [Finished :arrow_down:0.0% :arrow_up:0.0%] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/332d286d27744fa48687a8b362d54c47...08892a47d50741408503751417f8aad6/)
   [Finished :arrow_down:0.43% :arrow_up:0.0%] [test-mac-arm](https://conbench.ursa.dev/compare/runs/2ee70d70ee144544bddd1c65f9371324...60022d0bd781464599e7bd41acca41c5/)
   [Failed :arrow_down:0.36% :arrow_up:0.0%] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/33cd91ba5a344042a08924ad9b1f7012...5e00d43064db427dbd1dfa6fceca03da/)
   [Finished :arrow_down:0.2% :arrow_up:0.0%] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/97ec8bc4dd7c426ea2d1429aef335e4f...41acf638bde7420eb03855e77e7a8f35/)
   Buildkite builds:
   [Finished] [`760ad200` ec2-t3-xlarge-us-east-2](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/679)
   [Finished] [`760ad200` test-mac-arm](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/676)
   [Finished] [`760ad200` ursa-i9-9960x](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/665)
   [Finished] [`760ad200` ursa-thinkcentre-m75q](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/681)
   [Finished] [`3f2c33f5` ec2-t3-xlarge-us-east-2](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/678)
   [Finished] [`3f2c33f5` test-mac-arm](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/675)
   [Failed] [`3f2c33f5` ursa-i9-9960x](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/664)
   [Finished] [`3f2c33f5` ursa-thinkcentre-m75q](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/680)
   Supported benchmarks:
   ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
   test-mac-arm: Supported benchmark langs: C++, Python, R
   ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
   ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java
   


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

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

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

[GitHub] [arrow] davisusanibar commented on a diff in pull request #12941: ARROW-15755: [Java] Support Java 17

Posted by GitBox <gi...@apache.org>.
davisusanibar commented on code in PR #12941:
URL: https://github.com/apache/arrow/pull/12941#discussion_r855532845


##########
java/flight/flight-sql/src/main/java/org/apache/arrow/flight/sql/FlightSqlClient.java:
##########
@@ -346,7 +345,7 @@ public FlightInfo getExportedKeys(final TableRef tableRef, final CallOption... o
     }
 
     Objects.requireNonNull(tableRef.getTable());
-    builder.setTable(tableRef.getTable()).build();

Review Comment:
   Decided to move on your previous comment and remove the `build` but keep the rest of the line.



##########
java/flight/flight-core/src/test/java/org/apache/arrow/flight/TestClientMiddleware.java:
##########
@@ -84,6 +84,8 @@ public void testMultiValuedHeaders() {
         });
     // The server echoes the headers we send back to us, so ensure all the ones we sent are present with the correct
     // values in the correct order.
+    EXPECTED_BINARY_HEADERS.put("x-binary-bin", Arrays.asList(new byte[] {0}, new byte[]{1}));
+    EXPECTED_TEXT_HEADERS.put("x-text", Arrays.asList("foo", "bar"));

Review Comment:
   Changed



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

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

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

[GitHub] [arrow] davisusanibar commented on a diff in pull request #12941: ARROW-15755: [Java] Support Java 17

Posted by GitBox <gi...@apache.org>.
davisusanibar commented on code in PR #12941:
URL: https://github.com/apache/arrow/pull/12941#discussion_r854688625


##########
java/pom.xml:
##########
@@ -763,6 +766,60 @@
       </build>
     </profile>
 
+    <profile>
+      <id>error-prone-jdk11+</id>
+      <activation>
+        <jdk>[11,)</jdk>
+        <property>
+          <name>!m2e.version</name>
+        </property>
+      </activation>
+      <build>
+        <plugins>
+          <plugin>
+            <groupId>org.apache.maven.plugins</groupId>
+            <artifactId>maven-compiler-plugin</artifactId>
+            <configuration>
+              <source>8</source>
+              <target>8</target>
+              <encoding>UTF-8</encoding>
+              <compilerArgs combine.children="append">
+                <arg>-XDcompilePolicy=simple</arg>
+                <arg>
+                  -Xplugin:ErrorProne \
+                  -XepExcludedPaths:.*/(target/generated-sources)/.*

Review Comment:
   It does not work on that way.
   
   Be aware that when running on JDK 8 the flags cannot be wrapped across multiple lines. JDK 9 and above do allow the flags to be separated by newlines. That is, [the second <arg> element above can also be formatted as follows](https://errorprone.info/docs/flags) on JDK 9+, but not on JDK 8:
   
   



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

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

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

[GitHub] [arrow] toddfarmer commented on pull request #12941: ARROW-15755: [Java] Support Java 17

Posted by GitBox <gi...@apache.org>.
toddfarmer commented on PR #12941:
URL: https://github.com/apache/arrow/pull/12941#issuecomment-1113663251

   Just a note here as a reminder that https://arrow.apache.org/docs/dev/developers/java/building.html will need updates once support for additional JDK versions is added.


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

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

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

[GitHub] [arrow] davisusanibar commented on a diff in pull request #12941: ARROW-15755: [Java] Support Java 17

Posted by GitBox <gi...@apache.org>.
davisusanibar commented on code in PR #12941:
URL: https://github.com/apache/arrow/pull/12941#discussion_r874250276


##########
java/pom.xml:
##########
@@ -777,10 +781,40 @@
             <groupId>org.apache.maven.plugins</groupId>
             <artifactId>maven-compiler-plugin</artifactId>
             <configuration>
-              <fork>true</fork>
+              <source>8</source>
+              <target>8</target>
+              <encoding>UTF-8</encoding>
               <compilerArgs combine.children="append">
-                <arg>-J-Xbootclasspath/p:${settings.localRepository}/com/google/errorprone/javac/${errorprone.javac.version}/javac-${errorprone.javac.version}.jar</arg>
+                <arg>-XDcompilePolicy=simple</arg>
+                <arg>
+                  -Xplugin:ErrorProne \
+                  -XepExcludedPaths:.*/(target/generated-sources)/.*
+                </arg>
+                <arg>-J--add-exports=jdk.compiler/com.sun.tools.javac.api=ALL-UNNAMED</arg>
+                <arg>-J--add-exports=jdk.compiler/com.sun.tools.javac.file=ALL-UNNAMED</arg>
+                <arg>-J--add-exports=jdk.compiler/com.sun.tools.javac.main=ALL-UNNAMED</arg>
+                <arg>-J--add-exports=jdk.compiler/com.sun.tools.javac.model=ALL-UNNAMED</arg>
+                <arg>-J--add-exports=jdk.compiler/com.sun.tools.javac.parser=ALL-UNNAMED</arg>
+                <arg>-J--add-exports=jdk.compiler/com.sun.tools.javac.processing=ALL-UNNAMED</arg>
+                <arg>-J--add-exports=jdk.compiler/com.sun.tools.javac.tree=ALL-UNNAMED</arg>
+                <arg>-J--add-exports=jdk.compiler/com.sun.tools.javac.util=ALL-UNNAMED</arg>
+                <arg>-J--add-opens=jdk.compiler/com.sun.tools.javac.code=ALL-UNNAMED</arg>
+                <arg>-J--add-opens=jdk.compiler/com.sun.tools.javac.comp=ALL-UNNAMED</arg>

Review Comment:
   Hi @iamzafar as per the jar artifacts are exposed as a legacy mode (it means any jar are running in the classpath is consider as a unnamed module) and base on JEP 200 The Modular JDK: yes, this is needed:
   ```
   --add-opens ${module}/${package}=${target_modules}
   Let the ${target_modules} access all types and members, regardless of visibility, from ${module}'s ${package}
   ```
   Yes, this is an intermediate step: (1) Legacy mode (already implemented, (2) single module and (3) multi-module will be the last steps.
   
   This is a draft for implement JPMS and be more restrict about to not give ALL-UNNAMED access but there is a lot of discussion we need to close before to convert this draft into PR: https://github.com/apache/arrow/pull/13072. This is the document that I am going to send [for discussion to the ML](https://docs.google.com/document/d/1qcJ8LPm33UICuGjRnsGBcm8dLI08MyiL8BO5JVzTutA).
   



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

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

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

[GitHub] [arrow] lidavidm commented on a diff in pull request #12941: ARROW-15755: [Java] Support Java 17

Posted by GitBox <gi...@apache.org>.
lidavidm commented on code in PR #12941:
URL: https://github.com/apache/arrow/pull/12941#discussion_r854690601


##########
java/adapter/orc/pom.xml:
##########
@@ -52,10 +52,27 @@
                 </exclusion>
             </exclusions>
         </dependency>
+        <dependency>
+            <groupId>org.apache.hadoop</groupId>
+            <artifactId>hadoop-client-runtime</artifactId>
+            <version>3.3.2</version>
+            <scope>test</scope>
+            <exclusions>
+                <exclusion>
+                    <groupId>commons-logging</groupId>
+                    <artifactId>commons-logging</artifactId>

Review Comment:
   Though uh wow, that's been there since the initial Arrow commit in 2016. Are we sure Hadoop functions without this dependency, incidentally?



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

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

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

[GitHub] [arrow] davisusanibar commented on pull request #12941: ARROW-15755: [Java] Support Java 17

Posted by GitBox <gi...@apache.org>.
davisusanibar commented on PR #12941:
URL: https://github.com/apache/arrow/pull/12941#issuecomment-1105271354

   > You will want to rebase to get CI to run.
   > 
   > Can we add a CI job that tests JDK17?
   
   For CI jobs please take a look at https://issues.apache.org/jira/browse/ARROW-15177.  Thank for your support @raulcd 


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

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

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

[GitHub] [arrow] lwhite1 commented on pull request #12941: ARROW-15755: [Java] Support Java 17

Posted by GitBox <gi...@apache.org>.
lwhite1 commented on PR #12941:
URL: https://github.com/apache/arrow/pull/12941#issuecomment-1105201637

   @pitrou will do
   


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

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

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

[GitHub] [arrow] lidavidm commented on pull request #12941: ARROW-15755: [Java] Support Java 17

Posted by GitBox <gi...@apache.org>.
lidavidm commented on PR #12941:
URL: https://github.com/apache/arrow/pull/12941#issuecomment-1112144917

   Thanks @raulcd! I think we can merge this, then rebase your PR and check that it passes in CI, and then merge that as well?


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

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

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

[GitHub] [arrow] raulcd commented on pull request #12941: ARROW-15755: [Java] Support Java 17

Posted by GitBox <gi...@apache.org>.
raulcd commented on PR #12941:
URL: https://github.com/apache/arrow/pull/12941#issuecomment-1111970900

   Hi @davisusanibar I have created this draft PR to add Java 17 to CI (https://github.com/apache/arrow/pull/13021). As you can see from the triggered jobs, there is a new Java job:
   ![image](https://user-images.githubusercontent.com/639755/165702289-844ec524-3541-4613-8751-833c47e4912a.png)
   It is obviously failing (https://github.com/apache/arrow/runs/6208174355?check_suite_focus=true) because the fixes for Java 17 are on your PR but I have cherry-picked the commit locally on top of your branch and I have validated the build is working and tests are passing. Tested locally with:
   ```
   $ JDK=17 MAVEN=3.8.5 JDK_NAME=openjdk docker-compose run --rm debian-java
   ```
   I have validated java version was the correct when testing:
   ```
   bash-4.4# java --version
   openjdk 17.0.2 2022-01-18
   OpenJDK Runtime Environment (build 17.0.2+8-86)
   OpenJDK 64-Bit Server VM (build 17.0.2+8-86, mixed mode, sharing)
   bash-4.4# exit
   ```
   I am not sure how we do this but probably you might want to cherrypick my commit and apply it on your PR.


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

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

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

[GitHub] [arrow] lidavidm commented on a diff in pull request #12941: ARROW-15755: [Java] Support Java 17

Posted by GitBox <gi...@apache.org>.
lidavidm commented on code in PR #12941:
URL: https://github.com/apache/arrow/pull/12941#discussion_r855493600


##########
java/flight/flight-core/src/test/java/org/apache/arrow/flight/TestClientMiddleware.java:
##########
@@ -84,6 +84,8 @@ public void testMultiValuedHeaders() {
         });
     // The server echoes the headers we send back to us, so ensure all the ones we sent are present with the correct
     // values in the correct order.
+    EXPECTED_BINARY_HEADERS.put("x-binary-bin", Arrays.asList(new byte[] {0}, new byte[]{1}));
+    EXPECTED_TEXT_HEADERS.put("x-text", Arrays.asList("foo", "bar"));

Review Comment:
   Hmm, inline the declarations here too if they're only used in this test? Else put the initialization in a `static` block



##########
java/flight/flight-sql/src/main/java/org/apache/arrow/flight/sql/FlightSqlClient.java:
##########
@@ -346,7 +345,7 @@ public FlightInfo getExportedKeys(final TableRef tableRef, final CallOption... o
     }
 
     Objects.requireNonNull(tableRef.getTable());
-    builder.setTable(tableRef.getTable()).build();

Review Comment:
   It seems the right fix should be to just remove `.build`? Or else, inline it below or pull out some code? e.g. 
   
   `Any command = Any.pack(builder.setTable(...).build());`



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

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

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

[GitHub] [arrow] davisusanibar commented on a diff in pull request #12941: ARROW-15755: [Java] Support Java 17

Posted by GitBox <gi...@apache.org>.
davisusanibar commented on code in PR #12941:
URL: https://github.com/apache/arrow/pull/12941#discussion_r855528794


##########
java/flight/flight-sql/src/main/java/org/apache/arrow/flight/sql/FlightSqlClient.java:
##########
@@ -346,7 +345,7 @@ public FlightInfo getExportedKeys(final TableRef tableRef, final CallOption... o
     }
 
     Objects.requireNonNull(tableRef.getTable());
-    builder.setTable(tableRef.getTable()).build();

Review Comment:
   If I change to `Any command = Any.pack(builder.setTable(tableRef.getTable()).build());` it is working but I don't know what Any.pack is doing behind the scenes or if it is ok to use that instead of `initialize`method?



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

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

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

[GitHub] [arrow] lidavidm commented on pull request #12941: ARROW-15755: [Java] Support Java 17

Posted by GitBox <gi...@apache.org>.
lidavidm commented on PR #12941:
URL: https://github.com/apache/arrow/pull/12941#issuecomment-1113663753

   Good point. @davisusanibar can we include the docs changes here?


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

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

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

[GitHub] [arrow] lidavidm closed pull request #12941: ARROW-15755: [Java] Support Java 17

Posted by GitBox <gi...@apache.org>.
lidavidm closed pull request #12941: ARROW-15755: [Java] Support Java 17
URL: https://github.com/apache/arrow/pull/12941


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

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

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

[GitHub] [arrow] lidavidm commented on pull request #12941: ARROW-15755: [Java] Support Java 17

Posted by GitBox <gi...@apache.org>.
lidavidm commented on PR #12941:
URL: https://github.com/apache/arrow/pull/12941#issuecomment-1104589534

   You will want to rebase to get CI to run.
   
   Can we add a CI job that tests JDK17?


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

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

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

[GitHub] [arrow] lidavidm commented on a diff in pull request #12941: ARROW-15755: [Java] Support Java 17

Posted by GitBox <gi...@apache.org>.
lidavidm commented on code in PR #12941:
URL: https://github.com/apache/arrow/pull/12941#discussion_r854690039


##########
java/pom.xml:
##########
@@ -763,6 +766,60 @@
       </build>
     </profile>
 
+    <profile>
+      <id>error-prone-jdk11+</id>
+      <activation>
+        <jdk>[11,)</jdk>
+        <property>
+          <name>!m2e.version</name>
+        </property>
+      </activation>
+      <build>
+        <plugins>
+          <plugin>
+            <groupId>org.apache.maven.plugins</groupId>
+            <artifactId>maven-compiler-plugin</artifactId>
+            <configuration>
+              <source>8</source>
+              <target>8</target>
+              <encoding>UTF-8</encoding>
+              <compilerArgs combine.children="append">
+                <arg>-XDcompilePolicy=simple</arg>
+                <arg>
+                  -Xplugin:ErrorProne \
+                  -XepExcludedPaths:.*/(target/generated-sources)/.*

Review Comment:
   Ah, weird.



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

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

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

[GitHub] [arrow] davisusanibar commented on a diff in pull request #12941: ARROW-15755: [Java] Support Java 17

Posted by GitBox <gi...@apache.org>.
davisusanibar commented on code in PR #12941:
URL: https://github.com/apache/arrow/pull/12941#discussion_r856290120


##########
java/pom.xml:
##########
@@ -777,10 +781,40 @@
             <groupId>org.apache.maven.plugins</groupId>
             <artifactId>maven-compiler-plugin</artifactId>
             <configuration>
-              <fork>true</fork>
+              <source>8</source>
+              <target>8</target>
+              <encoding>UTF-8</encoding>
               <compilerArgs combine.children="append">
-                <arg>-J-Xbootclasspath/p:${settings.localRepository}/com/google/errorprone/javac/${errorprone.javac.version}/javac-${errorprone.javac.version}.jar</arg>
+                <arg>-XDcompilePolicy=simple</arg>
+                <arg>
+                  -Xplugin:ErrorProne \
+                  -XepExcludedPaths:.*/(target/generated-sources)/.*
+                </arg>
+                <arg>-J--add-exports=jdk.compiler/com.sun.tools.javac.api=ALL-UNNAMED</arg>
+                <arg>-J--add-exports=jdk.compiler/com.sun.tools.javac.file=ALL-UNNAMED</arg>
+                <arg>-J--add-exports=jdk.compiler/com.sun.tools.javac.main=ALL-UNNAMED</arg>
+                <arg>-J--add-exports=jdk.compiler/com.sun.tools.javac.model=ALL-UNNAMED</arg>
+                <arg>-J--add-exports=jdk.compiler/com.sun.tools.javac.parser=ALL-UNNAMED</arg>
+                <arg>-J--add-exports=jdk.compiler/com.sun.tools.javac.processing=ALL-UNNAMED</arg>
+                <arg>-J--add-exports=jdk.compiler/com.sun.tools.javac.tree=ALL-UNNAMED</arg>
+                <arg>-J--add-exports=jdk.compiler/com.sun.tools.javac.util=ALL-UNNAMED</arg>
+                <arg>-J--add-opens=jdk.compiler/com.sun.tools.javac.code=ALL-UNNAMED</arg>
+                <arg>-J--add-opens=jdk.compiler/com.sun.tools.javac.comp=ALL-UNNAMED</arg>

Review Comment:
   There are two part on this question:
   
   1. Changed for arrow side: Changes related to `--add-exports"` are needed to continue supporting erroProne base on JSE11+ [installation doc](https://errorprone.info/docs/installation). It mean you won't need this changes if you run arrow java building code without errorProne validation (mvn clean install -P-error-prone-jdk11+ ....)
   
   2. Changes as a user of arrow: If the user are planning to use Arrow with JSE17 is needed to pass modules needed. For example if I run cookbook for IO https://arrow.apache.org/cookbook/java/io.html it finished with an error mention `Unable to make field long java.nio.Buffer.address accessible: module java.base does not "opens java.nio" to unnamed module` for that reason as a user for JSE17 (not for arrow changes) is needed to add VM arguments as `-ea --add-opens=java.base/java.nio=ALL-UNNAMED` and it will finished without errors.
   
   Please let me know if you have some additional doubts.



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

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

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

[GitHub] [arrow] davisusanibar commented on a diff in pull request #12941: ARROW-15755: [Java] Support Java 17

Posted by GitBox <gi...@apache.org>.
davisusanibar commented on code in PR #12941:
URL: https://github.com/apache/arrow/pull/12941#discussion_r857906259


##########
java/pom.xml:
##########
@@ -777,10 +781,40 @@
             <groupId>org.apache.maven.plugins</groupId>
             <artifactId>maven-compiler-plugin</artifactId>
             <configuration>
-              <fork>true</fork>
+              <source>8</source>
+              <target>8</target>
+              <encoding>UTF-8</encoding>
               <compilerArgs combine.children="append">
-                <arg>-J-Xbootclasspath/p:${settings.localRepository}/com/google/errorprone/javac/${errorprone.javac.version}/javac-${errorprone.javac.version}.jar</arg>
+                <arg>-XDcompilePolicy=simple</arg>
+                <arg>
+                  -Xplugin:ErrorProne \
+                  -XepExcludedPaths:.*/(target/generated-sources)/.*
+                </arg>
+                <arg>-J--add-exports=jdk.compiler/com.sun.tools.javac.api=ALL-UNNAMED</arg>
+                <arg>-J--add-exports=jdk.compiler/com.sun.tools.javac.file=ALL-UNNAMED</arg>
+                <arg>-J--add-exports=jdk.compiler/com.sun.tools.javac.main=ALL-UNNAMED</arg>
+                <arg>-J--add-exports=jdk.compiler/com.sun.tools.javac.model=ALL-UNNAMED</arg>
+                <arg>-J--add-exports=jdk.compiler/com.sun.tools.javac.parser=ALL-UNNAMED</arg>
+                <arg>-J--add-exports=jdk.compiler/com.sun.tools.javac.processing=ALL-UNNAMED</arg>
+                <arg>-J--add-exports=jdk.compiler/com.sun.tools.javac.tree=ALL-UNNAMED</arg>
+                <arg>-J--add-exports=jdk.compiler/com.sun.tools.javac.util=ALL-UNNAMED</arg>
+                <arg>-J--add-opens=jdk.compiler/com.sun.tools.javac.code=ALL-UNNAMED</arg>
+                <arg>-J--add-opens=jdk.compiler/com.sun.tools.javac.comp=ALL-UNNAMED</arg>

Review Comment:
   > Can we add point (2) to the docs?
   
   Hi Team, just send this [PR to update Arrow JavaSE17/18 documentation](https://github.com/apache/arrow/pull/12990). Please if you could review that, thanks.



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

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

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

[GitHub] [arrow] raulcd commented on pull request #12941: ARROW-15755: [Java] Support Java 17

Posted by GitBox <gi...@apache.org>.
raulcd commented on PR #12941:
URL: https://github.com/apache/arrow/pull/12941#issuecomment-1112173434

   > Thanks @raulcd! I think we can merge this, then rebase your PR and check that it passes in CI, and then merge that as well?
   
   Thanks @lidavidm! that works for me, I'll be keeping an eye on this PR and will rebase mine once it is merged!


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

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

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

[GitHub] [arrow] iamzafar commented on a diff in pull request #12941: ARROW-15755: [Java] Support Java 17

Posted by GitBox <gi...@apache.org>.
iamzafar commented on code in PR #12941:
URL: https://github.com/apache/arrow/pull/12941#discussion_r874120732


##########
java/pom.xml:
##########
@@ -777,10 +781,40 @@
             <groupId>org.apache.maven.plugins</groupId>
             <artifactId>maven-compiler-plugin</artifactId>
             <configuration>
-              <fork>true</fork>
+              <source>8</source>
+              <target>8</target>
+              <encoding>UTF-8</encoding>
               <compilerArgs combine.children="append">
-                <arg>-J-Xbootclasspath/p:${settings.localRepository}/com/google/errorprone/javac/${errorprone.javac.version}/javac-${errorprone.javac.version}.jar</arg>
+                <arg>-XDcompilePolicy=simple</arg>
+                <arg>
+                  -Xplugin:ErrorProne \
+                  -XepExcludedPaths:.*/(target/generated-sources)/.*
+                </arg>
+                <arg>-J--add-exports=jdk.compiler/com.sun.tools.javac.api=ALL-UNNAMED</arg>
+                <arg>-J--add-exports=jdk.compiler/com.sun.tools.javac.file=ALL-UNNAMED</arg>
+                <arg>-J--add-exports=jdk.compiler/com.sun.tools.javac.main=ALL-UNNAMED</arg>
+                <arg>-J--add-exports=jdk.compiler/com.sun.tools.javac.model=ALL-UNNAMED</arg>
+                <arg>-J--add-exports=jdk.compiler/com.sun.tools.javac.parser=ALL-UNNAMED</arg>
+                <arg>-J--add-exports=jdk.compiler/com.sun.tools.javac.processing=ALL-UNNAMED</arg>
+                <arg>-J--add-exports=jdk.compiler/com.sun.tools.javac.tree=ALL-UNNAMED</arg>
+                <arg>-J--add-exports=jdk.compiler/com.sun.tools.javac.util=ALL-UNNAMED</arg>
+                <arg>-J--add-opens=jdk.compiler/com.sun.tools.javac.code=ALL-UNNAMED</arg>
+                <arg>-J--add-opens=jdk.compiler/com.sun.tools.javac.comp=ALL-UNNAMED</arg>

Review Comment:
   @davisusanibar It looks like this is a half-finished solution because VM arguments should be considered a temporary solution. Is it possible to use ARROW with JDK 17 and not have VM arguments? Every time I see posts with VM arguments I feel a little worried. In [JEP-261](https://openjdk.java.net/jeps/261) there's a warning about the usage of `--add-opens`:
   
   > The --add-exports and --add-opens options must be used with great care. You can use them to gain access to an internal API of a library module, or even of the JDK itself, but you do so at your own risk: If that internal API is changed or removed then your library or application will fail.
   
   



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

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

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

[GitHub] [arrow] lidavidm commented on a diff in pull request #12941: ARROW-15755: [Java] Support Java 17

Posted by GitBox <gi...@apache.org>.
lidavidm commented on code in PR #12941:
URL: https://github.com/apache/arrow/pull/12941#discussion_r860857542


##########
java/memory/memory-core/src/test/java/org/apache/arrow/memory/util/TestArrowBufPointer.java:
##########
@@ -111,6 +111,7 @@ public void testNullPointersHashCode() {
   }
 
   @Test
+  @SuppressWarnings("ReturnValueIgnored")

Review Comment:
   We can remove the suppression now right?



##########
java/pom.xml:
##########
@@ -690,7 +692,7 @@
 
   <profiles>
     <profile>
-      <id>java-8</id>
+      <id>java-doclint</id>

Review Comment:
   Should this be "nodoclint" or something?



##########
java/flight/flight-core/src/test/java/org/apache/arrow/flight/TestClientMiddleware.java:
##########
@@ -246,12 +246,13 @@ public FlightClientMiddleware onCallStarted(CallInfo info) {
   }
 
   // Used to test that middleware can send and receive multi-valued text and binary headers.
-  static final Map<String, List<byte[]>> EXPECTED_BINARY_HEADERS = new HashMap<String, List<byte[]>>() {{
-      put("x-binary-bin", Arrays.asList(new byte[] {0}, new byte[]{1}));
-    }};
-  static final Map<String, List<String>> EXPECTED_TEXT_HEADERS = new HashMap<String, List<String>>() {{
-      put("x-text", Arrays.asList("foo", "bar"));
-    }};
+  static final Map<String, List<byte[]>> EXPECTED_BINARY_HEADERS = new HashMap<String, List<byte[]>>();
+  static final Map<String, List<String>> EXPECTED_TEXT_HEADERS = new HashMap<String, List<String>>();
+
+  {

Review Comment:
   ```suggestion
     static {
   ```



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

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

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

[GitHub] [arrow] lidavidm commented on a diff in pull request #12941: ARROW-15755: [Java] Support Java 17

Posted by GitBox <gi...@apache.org>.
lidavidm commented on code in PR #12941:
URL: https://github.com/apache/arrow/pull/12941#discussion_r861206455


##########
java/flight/flight-core/src/test/java/org/apache/arrow/flight/TestClientMiddleware.java:
##########
@@ -246,12 +246,13 @@ public FlightClientMiddleware onCallStarted(CallInfo info) {
   }
 
   // Used to test that middleware can send and receive multi-valued text and binary headers.
-  static final Map<String, List<byte[]>> EXPECTED_BINARY_HEADERS = new HashMap<String, List<byte[]>>() {{
-      put("x-binary-bin", Arrays.asList(new byte[] {0}, new byte[]{1}));
-    }};
-  static final Map<String, List<String>> EXPECTED_TEXT_HEADERS = new HashMap<String, List<String>>() {{
-      put("x-text", Arrays.asList("foo", "bar"));
-    }};
+  static final Map<String, List<byte[]>> EXPECTED_BINARY_HEADERS = new HashMap<String, List<byte[]>>();
+  static final Map<String, List<String>> EXPECTED_TEXT_HEADERS = new HashMap<String, List<String>>();
+
+  {

Review Comment:
   Err, nothing changed here and this is a suggested addition - did you forget to add this?



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

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

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

[GitHub] [arrow] lidavidm commented on a diff in pull request #12941: ARROW-15755: [Java] Support Java 17

Posted by GitBox <gi...@apache.org>.
lidavidm commented on code in PR #12941:
URL: https://github.com/apache/arrow/pull/12941#discussion_r854690362


##########
java/adapter/orc/pom.xml:
##########
@@ -52,10 +52,27 @@
                 </exclusion>
             </exclusions>
         </dependency>
+        <dependency>
+            <groupId>org.apache.hadoop</groupId>
+            <artifactId>hadoop-client-runtime</artifactId>
+            <version>3.3.2</version>
+            <scope>test</scope>
+            <exclusions>
+                <exclusion>
+                    <groupId>commons-logging</groupId>
+                    <artifactId>commons-logging</artifactId>

Review Comment:
   Ah, I didn't realize we had a bannedDependencies set. Ok, it's probably fine to leave this as is then.



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

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

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

[GitHub] [arrow] lidavidm commented on pull request #12941: ARROW-15755: [Java] Support Java 17

Posted by GitBox <gi...@apache.org>.
lidavidm commented on PR #12941:
URL: https://github.com/apache/arrow/pull/12941#issuecomment-1105272439

   Well I don't think we should merge this without a CI job to actually validate it.


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

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

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

[GitHub] [arrow] davisusanibar commented on a diff in pull request #12941: ARROW-15755: [Java] Support Java 17

Posted by GitBox <gi...@apache.org>.
davisusanibar commented on code in PR #12941:
URL: https://github.com/apache/arrow/pull/12941#discussion_r855522488


##########
java/flight/flight-sql/src/main/java/org/apache/arrow/flight/sql/FlightSqlClient.java:
##########
@@ -346,7 +345,7 @@ public FlightInfo getExportedKeys(final TableRef tableRef, final CallOption... o
     }
 
     Objects.requireNonNull(tableRef.getTable());
-    builder.setTable(tableRef.getTable()).build();

Review Comment:
   If it is deleted `builder.setTable(tableRef.getTable()).build();`, errors appear like these:
   ````
   [ERROR] Failures: 
   [ERROR]   TestFlightSql.testGetCommandExportedKeys:589 expected:<1> but was:<0>
   [ERROR]   TestFlightSql.testGetCommandImportedKeys:619 expected:<1> but was:<0>
   [ERROR] Errors: 
   [ERROR]   TestFlightSql.testGetPrimaryKey:505 ยป IndexOutOfBounds Index 0 out of bounds f...
   [INFO] 
   [ERROR] Tests run: 47, Failures: 2, Errors: 1, Skipped: 0
   
   ````



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

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

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

[GitHub] [arrow] lidavidm commented on a diff in pull request #12941: ARROW-15755: [Java] Support Java 17

Posted by GitBox <gi...@apache.org>.
lidavidm commented on code in PR #12941:
URL: https://github.com/apache/arrow/pull/12941#discussion_r855531121


##########
java/flight/flight-sql/src/main/java/org/apache/arrow/flight/sql/FlightSqlClient.java:
##########
@@ -346,7 +345,7 @@ public FlightInfo getExportedKeys(final TableRef tableRef, final CallOption... o
     }
 
     Objects.requireNonNull(tableRef.getTable());
-    builder.setTable(tableRef.getTable()).build();

Review Comment:
   Can you clarify? `isInitialized` doesn't do anything, as the method documentation says. 



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

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

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

[GitHub] [arrow] davisusanibar commented on a diff in pull request #12941: ARROW-15755: [Java] Support Java 17

Posted by GitBox <gi...@apache.org>.
davisusanibar commented on code in PR #12941:
URL: https://github.com/apache/arrow/pull/12941#discussion_r855533095


##########
java/flight/flight-core/src/test/java/org/apache/arrow/flight/TestClientMiddleware.java:
##########
@@ -246,9 +246,11 @@ public FlightClientMiddleware onCallStarted(CallInfo info) {
   }
 
   // Used to test that middleware can send and receive multi-valued text and binary headers.
+  @SuppressWarnings({"DoubleBraceInitialization"})

Review Comment:
   Thanks, changed



##########
java/flight/flight-core/src/test/java/org/apache/arrow/flight/TestCallOptions.java:
##########
@@ -97,6 +97,7 @@ public void mixedProperties() {
     testHeaders(headers);
   }
 
+  @SuppressWarnings("ReturnValueIgnored")

Review Comment:
   Changed



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

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

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

[GitHub] [arrow] pitrou commented on pull request #12941: ARROW-15755: [Java] Support Java 17

Posted by GitBox <gi...@apache.org>.
pitrou commented on PR #12941:
URL: https://github.com/apache/arrow/pull/12941#issuecomment-1114855569

   Great milestone, thanks a lot @davisusanibar !


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

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

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