You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@orc.apache.org by "mystic-lama (via GitHub)" <gi...@apache.org> on 2023/08/13 15:20:47 UTC
[GitHub] [orc] mystic-lama opened a new pull request, #1581: ORC-1477: Remove unused imports from Test classes
mystic-lama opened a new pull request, #1581:
URL: https://github.com/apache/orc/pull/1581
### What changes were proposed in this pull request?
The PR aims to remove unused imports from following Test classes
- java/tools/src/test/org/apache/orc/tools/TestJsonFileDump.java
- java/mapreduce/src/test/org/apache/orc/mapreduce/TestMapreduceOrcOutputFormat.java
- java/core/src/test/org/apache/orc/TestMinSeekSize.java
- java/mapreduce/src/test/org/apache/orc/mapreduce/TestMrUnit.java
- java/core/src/test/org/apache/orc/TestOrcFilterContext.java
- java/mapreduce/src/test/org/apache/orc/mapred/TestOrcOutputFormat.java
- java/core/src/test/org/apache/orc/impl/filter/TestPluginFilters.java
- java/core/src/test/org/apache/orc/impl/filter/TestPluginFilterService.java
- java/core/src/test/org/apache/orc/impl/TestWriterImpl.java
**NOTE** - TestWriterImpl had * import, which was updated to use specific import
### Why are the changes needed?
Removing unused imports reduces clutter in code and make is easier to read, possibly avoiding unintended dependencies
### How was this patch tested?
Build and Test cases pass successfully
--
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: issues-unsubscribe@orc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] guiyanakuang commented on a diff in pull request #1581: ORC-1477: Remove unused imports from Test classes
Posted by "guiyanakuang (via GitHub)" <gi...@apache.org>.
guiyanakuang commented on code in PR #1581:
URL: https://github.com/apache/orc/pull/1581#discussion_r1292924336
##########
java/core/src/test/org/apache/orc/impl/TestWriterImpl.java:
##########
@@ -29,7 +29,7 @@
import org.apache.orc.Reader;
import org.apache.orc.TypeDescription;
import org.apache.orc.Writer;
-import org.apache.orc.*;
+import org.apache.orc.StripeInformation;
Review Comment:
You can try using the plugin https://plugins.jetbrains.com/plugin/1065-checkstyle-idea so that you can use checkstyle.xml from ORC project instead of IDEA's default code checking.
<img width="919" alt="image" src="https://github.com/apache/orc/assets/4069905/7375ac17-b446-4d25-87dd-5b55a182b91c">
I've been using it for a while and it works great.
https://github.com/apache/orc/blob/1c83941a213a55c1d465bae1c02e49e260f7b9cd/java/checkstyle.xml#L42
--
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: issues-unsubscribe@orc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] dongjoon-hyun commented on pull request #1581: ORC-1477: Remove unused imports from Test classes
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #1581:
URL: https://github.com/apache/orc/pull/1581#issuecomment-1679979911
Thank you, @guiyanakuang .
To make it sure that this PR fixes all test cases, let's enable it for `test/java` directories too, @mystic-lama , in this 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: issues-unsubscribe@orc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] guiyanakuang commented on pull request #1581: ORC-1477: Remove unused imports from Test classes
Posted by "guiyanakuang (via GitHub)" <gi...@apache.org>.
guiyanakuang commented on PR #1581:
URL: https://github.com/apache/orc/pull/1581#issuecomment-1681563493
Fixing the test code style takes a lot of effort, so I would suggest fixing different types of code style issues in multiple pr's.
--
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: issues-unsubscribe@orc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] mystic-lama commented on pull request #1581: ORC-1477: Remove unused imports from Test classes
Posted by "mystic-lama (via GitHub)" <gi...@apache.org>.
mystic-lama commented on PR #1581:
URL: https://github.com/apache/orc/pull/1581#issuecomment-1694766812
@dongjoon-hyun Thanks for the detailed explanation. I agree that this doesn't bring much value, hence I asked. Now that we are on same page, this task has been addressed :)
--
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: issues-unsubscribe@orc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] mystic-lama closed pull request #1581: ORC-1477: Remove unused imports from Test classes
Posted by "mystic-lama (via GitHub)" <gi...@apache.org>.
mystic-lama closed pull request #1581: ORC-1477: Remove unused imports from Test classes
URL: https://github.com/apache/orc/pull/1581
--
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: issues-unsubscribe@orc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] guiyanakuang commented on pull request #1581: ORC-1477: Remove unused imports from Test classes
Posted by "guiyanakuang (via GitHub)" <gi...@apache.org>.
guiyanakuang commented on PR #1581:
URL: https://github.com/apache/orc/pull/1581#issuecomment-1681544947
https://github.com/apache/orc/blob/b347df0d0254635f8e2b2bf310e959e84c36b133/java/pom.xml#L788-L815
These plugins are defined in the analyze profile, and we need to specify the profile to use them.
`./mvnw package -Panalyze`
--
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: issues-unsubscribe@orc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] mystic-lama commented on pull request #1581: ORC-1477: Remove unused imports from Test classes
Posted by "mystic-lama (via GitHub)" <gi...@apache.org>.
mystic-lama commented on PR #1581:
URL: https://github.com/apache/orc/pull/1581#issuecomment-1681553320
Thanks @guiyanakuang I completely forgot about profiles.
The results shows 1188 error's. Should create another JIRA to tackle this?
cc @dongjoon-hyun
```
bash-5.1$ ./mvnw package -Panalyze
....
[ERROR] src/test/org/apache/orc/TestRowFilteringComplexTypes.java:[324,9] (indentation) Indentation: 'try rcurly' has incorrect indentation level 8, expected level should be 4.
[ERROR] src/test/org/apache/orc/TestRowFilteringComplexTypes.java:[325,5] (indentation) Indentation: 'method def rcurly' has incorrect indentation level 4, expected level should be 2.
[ERROR] src/test/org/apache/orc/TestOrcConf.java:[27,1] (imports) CustomImportOrder: Wrong lexicographical order for 'org.junit.jupiter.api.Assertions.assertEquals' import. Should be before 'org.junit.jupiter.api.Assertions.assertFalse'.
[ERROR] src/test/org/apache/orc/TestUnicode.java:[147] (sizes) LineLength: Line is longer than 100 characters (found 109).
[INFO] ------------------------------------------------------------------------
[INFO] Reactor Summary for Apache ORC 2.0.0-SNAPSHOT:
[INFO]
[INFO] Apache ORC ......................................... SUCCESS [ 2.733 s]
[INFO] ORC Shims .......................................... SUCCESS [ 3.672 s]
[INFO] ORC Core ........................................... FAILURE [01:48 min]
[INFO] ORC MapReduce ...................................... SKIPPED
[INFO] ORC Tools .......................................... SKIPPED
[INFO] ORC Examples ....................................... SKIPPED
[INFO] ------------------------------------------------------------------------
[INFO] BUILD FAILURE
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 01:54 min
[INFO] Finished at: 2023-08-16T20:17:16-07:00
[INFO] ------------------------------------------------------------------------
[ERROR] Failed to execute goal org.apache.maven.plugins:maven-checkstyle-plugin:3.3.0:check (default) on project orc-core: You have 1188 Checkstyle violations. -> [Help 1]
```
--
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: issues-unsubscribe@orc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] guiyanakuang commented on pull request #1581: ORC-1477: Remove unused imports from Test classes
Posted by "guiyanakuang (via GitHub)" <gi...@apache.org>.
guiyanakuang commented on PR #1581:
URL: https://github.com/apache/orc/pull/1581#issuecomment-1679877695
https://github.com/apache/orc/blob/857cf7cce075407d611bdcdce69f9547b2ef4f29/java/pom.xml#L439-L445
I think it's because the configuration doesn't specify checking the test code.
--
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: issues-unsubscribe@orc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] mystic-lama commented on pull request #1581: ORC-1477: Remove unused imports from Test classes
Posted by "mystic-lama (via GitHub)" <gi...@apache.org>.
mystic-lama commented on PR #1581:
URL: https://github.com/apache/orc/pull/1581#issuecomment-1683238843
@dongjoon-hyun As of now there are no further unused imports, ran the inspection from IntelliJ and verified.
Perfectly fine refining the title, open to suggestions :)
I personally feel that we should enforce this on test classes too. I rely a lot on test code to understand things within ORC.
I do not know how much effort is involved right now, however I am happy to take a tab at it, one module at a time. Let me create a JIRA for 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: issues-unsubscribe@orc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] mystic-lama commented on pull request #1581: ORC-1477: Remove unused imports from Test classes
Posted by "mystic-lama (via GitHub)" <gi...@apache.org>.
mystic-lama commented on PR #1581:
URL: https://github.com/apache/orc/pull/1581#issuecomment-1681504538
I added following line, as mentioned above
```xml
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-checkstyle-plugin</artifactId>
<version>3.3.0</version>
<configuration>
<sourceDirectories>
<directory>${basedir}/src/java</directory>
<directory>${basedir}/src/test</directory>
</sourceDirectories>`
....
```
To validate that it works, added an unused import in test class, but unfortunately the build didn't fail. Used the usual package command. Am I missing something?
--
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: issues-unsubscribe@orc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] mystic-lama commented on pull request #1581: ORC-1477: Remove unused imports from Test classes
Posted by "mystic-lama (via GitHub)" <gi...@apache.org>.
mystic-lama commented on PR #1581:
URL: https://github.com/apache/orc/pull/1581#issuecomment-1684341315
@dongjoon-hyun Agree. Got your point now.
Let me close this 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: issues-unsubscribe@orc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] mystic-lama commented on pull request #1581: ORC-1477: Remove unused imports from Test classes
Posted by "mystic-lama (via GitHub)" <gi...@apache.org>.
mystic-lama commented on PR #1581:
URL: https://github.com/apache/orc/pull/1581#issuecomment-1681569852
Agree, this would mean we won't be able to enable checkstyle for test folders.
Let's wait for @dongjoon-hyun opinion on this, since he brought this up.
--
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: issues-unsubscribe@orc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] guiyanakuang commented on a diff in pull request #1581: ORC-1477: Remove unused imports from Test classes
Posted by "guiyanakuang (via GitHub)" <gi...@apache.org>.
guiyanakuang commented on code in PR #1581:
URL: https://github.com/apache/orc/pull/1581#discussion_r1292819458
##########
java/core/src/test/org/apache/orc/impl/TestWriterImpl.java:
##########
@@ -29,7 +29,7 @@
import org.apache.orc.Reader;
import org.apache.orc.TypeDescription;
import org.apache.orc.Writer;
-import org.apache.orc.*;
+import org.apache.orc.StripeInformation;
Review Comment:
It looks like the maven-checkstyle-plugin doesn't check the test code, but it's best to sort the imports in the group alphabetically
--
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: issues-unsubscribe@orc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] dongjoon-hyun commented on pull request #1581: ORC-1477: Remove unused imports from Test classes
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #1581:
URL: https://github.com/apache/orc/pull/1581#issuecomment-1694746092
To be clear, I don't say there is a regression when we didn't change anything. IIRC, we didn't delete any real code during refactoring. It means we didn't introduce any `Unused Imports` newly. In other words, you cannot call it the existing code as a `regression`.
> now that we have regression in place, shall we rework on this?
BTW, thank you for the proposal, @mystic-lama. As you know, Apache ORC is an open source project and every contributor can propose everything. We are not blocking anything. However, to protect Apache ORC project, the committers are able to give +1 for a few selected proposals. For this category, I decided to exclude all `Import` rules like the following because there is no real business logic in `Java import` statement. To simply put, although I understand your passion, let's call it a day by ignoring these import rules for now at 2.0.0 because we changed a lot already. We may want to re-try to introduce more cleaner environment at Apache ORC 2.1.0.
https://github.com/apache/orc/blob/81769064dc0e14aee0bc086a03e72d7a67047543/java/checkstyle-suppressions.xml#L45-L47
> Change Management Process is a way more organized in ORC and it's really good :)
--
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: issues-unsubscribe@orc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] mystic-lama commented on pull request #1581: ORC-1477: Remove unused imports from Test classes
Posted by "mystic-lama (via GitHub)" <gi...@apache.org>.
mystic-lama commented on PR #1581:
URL: https://github.com/apache/orc/pull/1581#issuecomment-1694709419
@dongjoon-hyun now that we have regression in place, shall we rework on this? I am asking coz, this change is trivial and there effort to backport changes to maintenance branches.
If we agree to work on this
Should we create a new PR for this or we reuse this (trying to understand how things work here).
Change Management Process is a way more organized in ORC :)
--
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: issues-unsubscribe@orc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] mystic-lama commented on a diff in pull request #1581: ORC-1477: Remove unused imports from Test classes
Posted by "mystic-lama (via GitHub)" <gi...@apache.org>.
mystic-lama commented on code in PR #1581:
URL: https://github.com/apache/orc/pull/1581#discussion_r1292823450
##########
java/core/src/test/org/apache/orc/impl/TestWriterImpl.java:
##########
@@ -29,7 +29,7 @@
import org.apache.orc.Reader;
import org.apache.orc.TypeDescription;
import org.apache.orc.Writer;
-import org.apache.orc.*;
+import org.apache.orc.StripeInformation;
Review Comment:
@guiyanakuang Thanks for the feedback. Shall be aware of this in future. I was hesitant in using Intellij Optimize imports, as it was changing the static import order too.
Updated the code to reflect the order.
--
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: issues-unsubscribe@orc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] guiyanakuang commented on a diff in pull request #1581: ORC-1477: Remove unused imports from Test classes
Posted by "guiyanakuang (via GitHub)" <gi...@apache.org>.
guiyanakuang commented on code in PR #1581:
URL: https://github.com/apache/orc/pull/1581#discussion_r1292924336
##########
java/core/src/test/org/apache/orc/impl/TestWriterImpl.java:
##########
@@ -29,7 +29,7 @@
import org.apache.orc.Reader;
import org.apache.orc.TypeDescription;
import org.apache.orc.Writer;
-import org.apache.orc.*;
+import org.apache.orc.StripeInformation;
Review Comment:
You can try using the plugin https://plugins.jetbrains.com/plugin/1065-checkstyle-idea so that you can use checkstyle.xml from your ORC project instead of IDEA's default code checking.
<img width="919" alt="image" src="https://github.com/apache/orc/assets/4069905/7375ac17-b446-4d25-87dd-5b55a182b91c">
I've been using it for a while and it works great.
https://github.com/apache/orc/blob/1c83941a213a55c1d465bae1c02e49e260f7b9cd/java/checkstyle.xml#L42
--
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: issues-unsubscribe@orc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org