You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@orc.apache.org by "dongjoon-hyun (via GitHub)" <gi...@apache.org> on 2023/08/26 05:12:36 UTC
[GitHub] [orc] dongjoon-hyun opened a new pull request, #1600: ORC-1486: Fixes checkstyle violation in test classes for orc-core module
dongjoon-hyun opened a new pull request, #1600:
URL: https://github.com/apache/orc/pull/1600
### What changes were proposed in this pull request?
This PR fixes checkstyle violation in test sources in orc-core module
### Why are the changes needed?
The change shall ensure our test code adhere's to same formatting as our source
### How was this patch tested?
Remove the following and run Checkstyle on `core` module.
https://github.com/apache/orc/blob/7787669c444d0cf18ba91effbba34b5608370b5b/java/checkstyle-suppressions.xml#L31-L36
```
$ cd java
$ mvn checkstyle:check --pl core
```
This closes #1590
--
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 #1600: ORC-1486: Fixes checkstyle violation in test classes for `orc-core` module
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #1600:
URL: https://github.com/apache/orc/pull/1600#issuecomment-1694565036
Here is the PR.
#1601
--
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 #1600: ORC-1486: Fixes checkstyle violation in test classes for orc-core module
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #1600:
URL: https://github.com/apache/orc/pull/1600#issuecomment-1694168378
cc @mystic-lama
--
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 a diff in pull request #1600: ORC-1486: Fixes checkstyle violation in test classes for `orc-core` module
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #1600:
URL: https://github.com/apache/orc/pull/1600#discussion_r1306589126
##########
java/core/src/test/org/apache/orc/TestNewIntegerEncoding.java:
##########
@@ -335,9 +335,9 @@ public void testBasicDelta4(OrcFile.EncodingStrategy encodingStrategy) throws Ex
public void testDeltaOverflow() throws Exception {
TypeDescription schema = TypeDescription.createLong();
- long[] inp = new long[]{4513343538618202719l, 4513343538618202711l,
- 2911390882471569739l,
- -9181829309989854913l};
+ long[] inp = new long[]{4513343538618202719L, 4513343538618202711L,
+ 2911390882471569739L,
+ -9181829309989854913L};
Review Comment:
May I asked which rule complains about that, @mystic-lama ?
As you see, this PR aims to introduce the minimal changes which means to touch only `Checkstyle` really complains. The verification is described in the PR description, too.
To simply put, if we mix different themes arbitrarily like that, it tends to make a mistake because of the human error factor. It makes reviewers difficulty to review the PRs. It ends up as a situation we cannot merge it.
Of course, we can take care of it later independently.
--
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 #1600: ORC-1486: Fixes checkstyle violation in test classes for `orc-core` module
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #1600:
URL: https://github.com/apache/orc/pull/1600#issuecomment-1694563862
Let me make a PR for the rest of module. It seems that we can close this checkstyle task today.
--
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 #1600: ORC-1486: Fixes checkstyle violation in test classes for `orc-core` module
Posted by "mystic-lama (via GitHub)" <gi...@apache.org>.
mystic-lama commented on PR #1600:
URL: https://github.com/apache/orc/pull/1600#issuecomment-1694486376
@dongjoon-hyun Thank you so much. I only get time during weekend or evening to work on OSS. Sadly got pulled into work, which took that time. I shall try to close things faster. Let me catch up with things and we can get this done ASAP
--
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 #1600: ORC-1486: Fixes checkstyle violation in test classes for `orc-core` module
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #1600:
URL: https://github.com/apache/orc/pull/1600#issuecomment-1694552752
BTW, please note that Apache ORC has 3 year maintenance policy. It means ORC committers need to maintain `branch-1.9` and `branch-1.8` and `branch-1.7` for three years and usually it requires us to backport from main to `branch-1.9` to `branch-1.8` and `branch-1.7` sequentially if needed. So, we are unable to accept a drastic change like your previous PR even for one time activity. It will tease us for 3 years.
--
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 closed pull request #1600: ORC-1486: Fixes checkstyle violation in test classes for `orc-core` module
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun closed pull request #1600: ORC-1486: Fixes checkstyle violation in test classes for `orc-core` module
URL: https://github.com/apache/orc/pull/1600
--
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 #1600: ORC-1486: Fixes checkstyle violation in test classes for `orc-core` module
Posted by "mystic-lama (via GitHub)" <gi...@apache.org>.
mystic-lama commented on code in PR #1600:
URL: https://github.com/apache/orc/pull/1600#discussion_r1306587737
##########
java/core/src/test/org/apache/orc/TestNewIntegerEncoding.java:
##########
@@ -335,9 +335,9 @@ public void testBasicDelta4(OrcFile.EncodingStrategy encodingStrategy) throws Ex
public void testDeltaOverflow() throws Exception {
TypeDescription schema = TypeDescription.createLong();
- long[] inp = new long[]{4513343538618202719l, 4513343538618202711l,
- 2911390882471569739l,
- -9181829309989854913l};
+ long[] inp = new long[]{4513343538618202719L, 4513343538618202711L,
+ 2911390882471569739L,
+ -9181829309989854913L};
Review Comment:
Shall we have this block like this? I find it more easy on my eyes. We don't have to fix these things now. I can take care of this later
```
long[] inp = new long[]{4513343538618202719L, 4513343538618202711L,
2911390882471569739L, -9181829309989854913L};
```
--
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 #1600: ORC-1486: Fixes checkstyle violation in test classes for `orc-core` module
Posted by "mystic-lama (via GitHub)" <gi...@apache.org>.
mystic-lama commented on code in PR #1600:
URL: https://github.com/apache/orc/pull/1600#discussion_r1306688059
##########
java/core/src/test/org/apache/orc/TestNewIntegerEncoding.java:
##########
@@ -335,9 +335,9 @@ public void testBasicDelta4(OrcFile.EncodingStrategy encodingStrategy) throws Ex
public void testDeltaOverflow() throws Exception {
TypeDescription schema = TypeDescription.createLong();
- long[] inp = new long[]{4513343538618202719l, 4513343538618202711l,
- 2911390882471569739l,
- -9181829309989854913l};
+ long[] inp = new long[]{4513343538618202719L, 4513343538618202711L,
+ 2911390882471569739L,
+ -9181829309989854913L};
Review Comment:
I agree with your point. I don't recall the rule, and did not mean to fix it in this PR
This is more for future, when we make code changes. I shall keep an eye for this and next time it hits, may be we can refine the rule.
--
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 #1600: ORC-1486: Fixes checkstyle violation in test classes for `orc-core` module
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #1600:
URL: https://github.com/apache/orc/pull/1600#issuecomment-1694553041
Merged to main for Apache ORC 2.0.
```
$ git log -n1 | tail -n2
Authored-by: mystic-lama <my...@gmail.com>
Signed-off-by: Dongjoon Hyun <do...@apache.org>
```
--
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 #1600: ORC-1486: Fixes checkstyle violation in test classes for `orc-core` module
Posted by "mystic-lama (via GitHub)" <gi...@apache.org>.
mystic-lama commented on PR #1600:
URL: https://github.com/apache/orc/pull/1600#issuecomment-1694702270
> BTW, please note that Apache ORC has 3 year maintenance policy. It means ORC committers need to maintain `branch-1.9` and `branch-1.8` and `branch-1.7` for three years and usually it requires us to backport from main to `branch-1.9` to `branch-1.8` and `branch-1.7` sequentially if needed. So, we are unable to accept a drastic change like your previous PR even for one time activity. It will tease us for 3 years.
@dongjoon-hyun This is good to know.
--
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