You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "vibhatha (via GitHub)" <gi...@apache.org> on 2024/09/05 05:12:55 UTC
[PR] GH-43962: [Java] Consider warnings as errors for Adapter Module [arrow]
vibhatha opened a new pull request, #43963:
URL: https://github.com/apache/arrow/pull/43963
### Rationale for this change
This PR configs the build such that warnings are considered as errors in the Adapter module. And corresponding code changes have also been made.
### What changes are included in this PR?
Adding flags to consider warnings as errors in javac and fixing the corresponding errors.
### Are these changes tested?
Tested by existing test cases.
### Are there any user-facing changes?
N/A
--
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
Re: [PR] GH-43962: [Java] Consider warnings as errors for Adapter Module [arrow]
Posted by "vibhatha (via GitHub)" <gi...@apache.org>.
vibhatha commented on PR #43963:
URL: https://github.com/apache/arrow/pull/43963#issuecomment-2333043434
@lidavidm I am planning to fix the warnings to errors per module at the moment and finally move the flag to the parent pom as we covered the code formatting. Is this approach okay?
--
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
Re: [PR] GH-43962: [Java] Consider warnings as errors for Adapter Module [arrow]
Posted by "vibhatha (via GitHub)" <gi...@apache.org>.
vibhatha commented on PR #43963:
URL: https://github.com/apache/arrow/pull/43963#issuecomment-2337171391
@lidavidm this is ready for review.
--
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
Re: [PR] GH-43962: [Java] Consider warnings as errors for Adapter Module [arrow]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #43963:
URL: https://github.com/apache/arrow/pull/43963#discussion_r1749605017
##########
java/adapter/orc/pom.xml:
##########
@@ -160,6 +160,39 @@ under the License.
</execution>
</executions>
</plugin>
+ <plugin>
+ <groupId>org.apache.maven.plugins</groupId>
+ <artifactId>maven-compiler-plugin</artifactId>
+ <configuration combine.self="override">
Review Comment:
can we possibly simplify this with `combine.children="append"`?
--
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
Re: [PR] GH-43962: [Java] Consider warnings as errors for Adapter Module [arrow]
Posted by "vibhatha (via GitHub)" <gi...@apache.org>.
vibhatha commented on code in PR #43963:
URL: https://github.com/apache/arrow/pull/43963#discussion_r1749929219
##########
java/adapter/avro/pom.xml:
##########
@@ -56,4 +56,30 @@ under the License.
<version>${dep.avro.version}</version>
</dependency>
</dependencies>
+
+ <build>
+ <plugins>
+ <plugin>
+ <groupId>org.apache.maven.plugins</groupId>
+ <artifactId>maven-compiler-plugin</artifactId>
+ <configuration combine.self="append">
Review Comment:
Yes, it seems to be the best. I updated, let's wait for the CIs to make sure.
--
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
Re: [PR] GH-43962: [Java] Consider warnings as errors for Adapter Module [arrow]
Posted by "vibhatha (via GitHub)" <gi...@apache.org>.
vibhatha commented on code in PR #43963:
URL: https://github.com/apache/arrow/pull/43963#discussion_r1749657594
##########
java/adapter/orc/pom.xml:
##########
@@ -160,6 +160,39 @@ under the License.
</execution>
</executions>
</plugin>
+ <plugin>
+ <groupId>org.apache.maven.plugins</groupId>
+ <artifactId>maven-compiler-plugin</artifactId>
+ <configuration combine.self="override">
Review Comment:
I updated, lets check the CIs (locally passing)
--
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
Re: [PR] GH-43962: [Java] Consider warnings as errors for Adapter Module [arrow]
Posted by "conbench-apache-arrow[bot] (via GitHub)" <gi...@apache.org>.
conbench-apache-arrow[bot] commented on PR #43963:
URL: https://github.com/apache/arrow/pull/43963#issuecomment-2344049837
After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit 2a793d6410539cf72f49f0bd06e1087d56c99d9f.
There were 4 benchmark results indicating a performance regression:
- Commit Run on `amd64-m5-4xlarge-linux` at [2024-09-11 06:53:51Z](https://conbench.ursa.dev/compare/runs/d9a16b92652e4b28b5d6a90f8e485157...4bd86a563f194b89a04de319eda32f03/)
- [`dataframe-to-table` (Python) with dataset=type_nested](https://conbench.ursa.dev/compare/benchmarks/066e0ea6cde172528000e741d7ca7a6e...066e14e41692701d8000db74c11ffedc)
- [`partitioned-dataset-filter` (R) with dataset=dataset-taxi-parquet, language=R, query=vignette](https://conbench.ursa.dev/compare/benchmarks/066e10b8e3697a188000458b9ebe7923...066e16d1da4c7da280004ec8872172db)
- and 2 more (see the report linked below)
The [full Conbench report](https://github.com/apache/arrow/runs/30003415175) has more details. It also includes information about 15 possible false positives for unstable benchmarks that are known to sometimes produce 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
Re: [PR] GH-43962: [Java] Consider warnings as errors for Adapter Module [arrow]
Posted by "vibhatha (via GitHub)" <gi...@apache.org>.
vibhatha commented on code in PR #43963:
URL: https://github.com/apache/arrow/pull/43963#discussion_r1752062802
##########
java/adapter/jdbc/pom.xml:
##########
@@ -116,6 +116,15 @@ under the License.
<argLine>--add-reads=org.apache.arrow.adapter.jdbc=com.fasterxml.jackson.dataformat.yaml --add-opens=java.base/java.nio=org.apache.arrow.memory.core,ALL-UNNAMED -Duser.timezone=UTC</argLine>
</configuration>
</plugin>
+ <plugin>
+ <groupId>org.apache.maven.plugins</groupId>
+ <artifactId>maven-compiler-plugin</artifactId>
+ <configuration combine.self="append">
Review Comment:
Fixed.
--
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
Re: [PR] GH-43962: [Java] Consider warnings as errors for Adapter Module [arrow]
Posted by "vibhatha (via GitHub)" <gi...@apache.org>.
vibhatha commented on code in PR #43963:
URL: https://github.com/apache/arrow/pull/43963#discussion_r1749618633
##########
java/adapter/orc/pom.xml:
##########
@@ -160,6 +160,39 @@ under the License.
</execution>
</executions>
</plugin>
+ <plugin>
+ <groupId>org.apache.maven.plugins</groupId>
+ <artifactId>maven-compiler-plugin</artifactId>
+ <configuration combine.self="override">
Review Comment:
Right, let me check.
--
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
Re: [PR] GH-43962: [Java] Consider warnings as errors for Adapter Module [arrow]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #43963:
URL: https://github.com/apache/arrow/pull/43963#discussion_r1749675353
##########
java/adapter/avro/pom.xml:
##########
@@ -56,4 +56,30 @@ under the License.
<version>${dep.avro.version}</version>
</dependency>
</dependencies>
+
+ <build>
+ <plugins>
+ <plugin>
+ <groupId>org.apache.maven.plugins</groupId>
+ <artifactId>maven-compiler-plugin</artifactId>
+ <configuration combine.self="append">
Review Comment:
Append should mean that none of these other arguments are required, right? Only compilerArgs?
--
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
Re: [PR] GH-43962: [Java] Consider warnings as errors for Adapter Module [arrow]
Posted by "danepitkin (via GitHub)" <gi...@apache.org>.
danepitkin commented on code in PR #43963:
URL: https://github.com/apache/arrow/pull/43963#discussion_r1750898507
##########
java/adapter/jdbc/pom.xml:
##########
@@ -116,6 +116,15 @@ under the License.
<argLine>--add-reads=org.apache.arrow.adapter.jdbc=com.fasterxml.jackson.dataformat.yaml --add-opens=java.base/java.nio=org.apache.arrow.memory.core,ALL-UNNAMED -Duser.timezone=UTC</argLine>
</configuration>
</plugin>
+ <plugin>
+ <groupId>org.apache.maven.plugins</groupId>
+ <artifactId>maven-compiler-plugin</artifactId>
+ <configuration combine.self="append">
Review Comment:
```suggestion
<configuration combine.children="append">
```
--
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
Re: [PR] GH-43962: [Java] Consider warnings as errors for Adapter Module [arrow]
Posted by "vibhatha (via GitHub)" <gi...@apache.org>.
vibhatha commented on code in PR #43963:
URL: https://github.com/apache/arrow/pull/43963#discussion_r1752055241
##########
java/adapter/jdbc/pom.xml:
##########
@@ -116,6 +116,15 @@ under the License.
<argLine>--add-reads=org.apache.arrow.adapter.jdbc=com.fasterxml.jackson.dataformat.yaml --add-opens=java.base/java.nio=org.apache.arrow.memory.core,ALL-UNNAMED -Duser.timezone=UTC</argLine>
</configuration>
</plugin>
+ <plugin>
+ <groupId>org.apache.maven.plugins</groupId>
+ <artifactId>maven-compiler-plugin</artifactId>
+ <configuration combine.self="append">
Review Comment:
Oh sorry, I missed this. Adding it 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
Re: [PR] GH-43962: [Java] Consider warnings as errors for Adapter Module [arrow]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm merged PR #43963:
URL: https://github.com/apache/arrow/pull/43963
--
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
Re: [PR] GH-43962: [Java] Consider warnings as errors for Adapter Module [arrow]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #43963:
URL: https://github.com/apache/arrow/pull/43963#discussion_r1749675555
##########
java/adapter/avro/pom.xml:
##########
@@ -56,4 +56,30 @@ under the License.
<version>${dep.avro.version}</version>
</dependency>
</dependencies>
+
+ <build>
+ <plugins>
+ <plugin>
+ <groupId>org.apache.maven.plugins</groupId>
+ <artifactId>maven-compiler-plugin</artifactId>
+ <configuration combine.self="append">
Review Comment:
Also should it be `combine.self` or `combine.children`?
--
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