You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@orc.apache.org by GitBox <gi...@apache.org> on 2021/09/29 23:18:33 UTC
[GitHub] [orc] autumnust opened a new pull request #922: ORC-1013: Renaming the object of WriterContext in all TreeWriter's derived class
autumnust opened a new pull request #922:
URL: https://github.com/apache/orc/pull/922
<!--
Thanks for sending a pull request! Here are some tips for you:
1. File a JIRA issue first and use it as a prefix of your PR title, e.g., `ORC-001: Fix ABC`.
2. Use your PR title to summarize what this PR proposes instead of describing the problem.
3. Make PR title and description complete because these will be the permanent commit log.
4. If possible, provide a concise and reproducible example to reproduce the issue for a faster review.
5. If the PR is unfinished, use GitHub PR Draft feature.
-->
### What changes were proposed in this pull request?
This is a simple change: Through reading the source code recently for doing another changes, I renamed the instance of `WriterContext` from `writer` into `writerContext`. The existing naming of the object seems misleading. The scope of this refactoring is small as the parameter is only used within the method itself.
There's also a comment change in the `OrcFile` to reflect current status.
### Why are the changes needed?
<!--
Please clarify why the changes are needed. For instance,
1. If you propose a new API, clarify the use case for a new API.
2. If you fix a bug, you can clarify why it is a bug.
-->
For better readability.
### How was this patch tested?
<!--
If tests were added, say they were added here. Please make sure to add some test cases that check the changes thoroughly including negative and positive cases if possible.
If it was tested in a way different from regular unit tests, please clarify how you tested step by step, ideally copy and paste-able, so that other reviewers can test and check, and descendants can verify in the future.
If tests were not added, please describe why they were not added and/or why it was difficult to add.
-->
There's no function change so passing the existing unit tests should be sufficient.
--
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: dev-unsubscribe@orc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] autumnust commented on pull request #922: ORC-1013: Renaming the object of WriterContext in all TreeWriter's derived class
Posted by GitBox <gi...@apache.org>.
autumnust commented on pull request #922:
URL: https://github.com/apache/orc/pull/922#issuecomment-937966539
> The main branch is fixed. Could you rebase your PR, @autumnust ?
Sorry I was pulled into a bunch of other issues recently. The CI looks good, should be ready for merging 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: dev-unsubscribe@orc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] dongjoon-hyun merged pull request #922: ORC-1013: Renaming a parameter in constructors of TreeWriter's derived classes
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun merged pull request #922:
URL: https://github.com/apache/orc/pull/922
--
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: dev-unsubscribe@orc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] dongjoon-hyun commented on a change in pull request #922: ORC-1013: Renaming the object of WriterContext in all TreeWriter's derived class
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on a change in pull request #922:
URL: https://github.com/apache/orc/pull/922#discussion_r719059436
##########
File path: java/core/src/java/org/apache/orc/OrcFile.java
##########
@@ -691,7 +691,7 @@ public WriterOptions physicalWriter(PhysicalWriter writer) {
}
/**
- * A package local option to set the memory manager.
+ * A public option to set the memory manager.
Review comment:
We usually don't mix like this. Especially, this minor change exists in an independent file. Please revert this. You can do this later in another 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: dev-unsubscribe@orc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] autumnust commented on a change in pull request #922: ORC-1013: Renaming the object of WriterContext in all TreeWriter's derived class
Posted by GitBox <gi...@apache.org>.
autumnust commented on a change in pull request #922:
URL: https://github.com/apache/orc/pull/922#discussion_r719082413
##########
File path: java/core/src/java/org/apache/orc/impl/OutStream.java
##########
@@ -399,4 +399,3 @@ public void suppress() {
receiver.suppress();
}
}
-
Review comment:
yes I think it is already updated from the newer commit as leaving last line of a file blank is actually required by the checkstyle.
--
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: dev-unsubscribe@orc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] autumnust commented on pull request #922: ORC-1013: Renaming the object of WriterContext in all TreeWriter's derived class
Posted by GitBox <gi...@apache.org>.
autumnust commented on pull request #922:
URL: https://github.com/apache/orc/pull/922#issuecomment-937966539
> The main branch is fixed. Could you rebase your PR, @autumnust ?
Sorry I was pulled into a bunch of other issues recently. The CI looks good, should be ready for merging 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: dev-unsubscribe@orc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] dongjoon-hyun edited a comment on pull request #922: ORC-1013: Renaming the object of WriterContext in all TreeWriter's derived class
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun edited a comment on pull request #922:
URL: https://github.com/apache/orc/pull/922#issuecomment-930787150
- To @autumnust . Thank you for making a PR. I agree with your intention.
- To @guiyanakuang . It's okay to approve this if you think it looks good to you. To become a committer, you had better practice to press `Approve` button with `LGTM`. BTW, it shows more responsibility of course. :)
--
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: dev-unsubscribe@orc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] dongjoon-hyun commented on a change in pull request #922: ORC-1013: Renaming the object of WriterContext in all TreeWriter's derived class
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on a change in pull request #922:
URL: https://github.com/apache/orc/pull/922#discussion_r719811261
##########
File path: java/core/src/java/org/apache/orc/OrcFile.java
##########
@@ -691,7 +691,7 @@ public WriterOptions physicalWriter(PhysicalWriter writer) {
}
/**
- * A package local option to set the memory manager.
+ * A public option to set the memory manager.
Review comment:
Why do you want to blocked by another PR, @autumnust ?
--
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: dev-unsubscribe@orc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] autumnust commented on a change in pull request #922: ORC-1013: Renaming the object of WriterContext in all TreeWriter's derived class
Posted by GitBox <gi...@apache.org>.
autumnust commented on a change in pull request #922:
URL: https://github.com/apache/orc/pull/922#discussion_r719753938
##########
File path: java/core/src/java/org/apache/orc/OrcFile.java
##########
@@ -691,7 +691,7 @@ public WriterOptions physicalWriter(PhysicalWriter writer) {
}
/**
- * A package local option to set the memory manager.
+ * A public option to set the memory manager.
Review comment:
I raised another PR for this line: https://github.com/apache/orc/pull/923
I can rebase against the upstream once that's merged in.
--
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: dev-unsubscribe@orc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] dongjoon-hyun commented on a change in pull request #922: ORC-1013: Renaming the object of WriterContext in all TreeWriter's derived class
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on a change in pull request #922:
URL: https://github.com/apache/orc/pull/922#discussion_r719811261
##########
File path: java/core/src/java/org/apache/orc/OrcFile.java
##########
@@ -691,7 +691,7 @@ public WriterOptions physicalWriter(PhysicalWriter writer) {
}
/**
- * A package local option to set the memory manager.
+ * A public option to set the memory manager.
Review comment:
May I ask you the reason why you want to be blocked by another PR, @autumnust ?
--
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: dev-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 #922: ORC-1013: Renaming the object of WriterContext in all TreeWriter's derived class
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on pull request #922:
URL: https://github.com/apache/orc/pull/922#issuecomment-932694797
@autumnust . I cannot merge this because of https://github.com/apache/orc/pull/922/files#diff-4c43c10da88510010bb511ba6e6892bb5ea6cc29be1b0b9d97c3bbaae5d382e2R694 .
As you see here, we had better keep each PR independently next time. If you start to lock something like this, it degrades the community throughput because we are working asynchronously. :)
--
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: dev-unsubscribe@orc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] guiyanakuang commented on pull request #922: ORC-1013: Renaming the object of WriterContext in all TreeWriter's derived class
Posted by GitBox <gi...@apache.org>.
guiyanakuang commented on pull request #922:
URL: https://github.com/apache/orc/pull/922#issuecomment-930793032
@dongjoon-hyun Thanks for the reminder, I get it, the committer needs to take more responsibility.
--
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: dev-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 #922: ORC-1013: Renaming the object of WriterContext in all TreeWriter's derived class
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on pull request #922:
URL: https://github.com/apache/orc/pull/922#issuecomment-934103348
The main branch is fixed. Could you rebase your PR, @autumnust ?
--
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: dev-unsubscribe@orc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] autumnust commented on pull request #922: ORC-1013: Renaming the object of WriterContext in all TreeWriter's derived class
Posted by GitBox <gi...@apache.org>.
autumnust commented on pull request #922:
URL: https://github.com/apache/orc/pull/922#issuecomment-930810172
> If we want to change this, I prefer to use `context` instead of writerContext`because it's more consistent with the base class,`TreeWriterBase`.
>
> ```
> TreeWriterBase(TypeDescription schema,
> WriterEncryptionVariant encryption,
> WriterContext context) throws IOException {
> ```
agree. Will address.
--
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: dev-unsubscribe@orc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] dongjoon-hyun commented on a change in pull request #922: ORC-1013: Renaming the object of WriterContext in all TreeWriter's derived class
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on a change in pull request #922:
URL: https://github.com/apache/orc/pull/922#discussion_r719059530
##########
File path: java/core/src/java/org/apache/orc/impl/OutStream.java
##########
@@ -399,4 +399,3 @@ public void suppress() {
receiver.suppress();
}
}
-
Review comment:
Ditto. Please revert this from 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: dev-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 #922: ORC-1013: Renaming the object of WriterContext in all TreeWriter's derived class
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on pull request #922:
URL: https://github.com/apache/orc/pull/922#issuecomment-930787150
- To @autumnust . Thank you for making a PR. I agree with your intention.
- To @guiyanakuang . It's okay to approve this if you think it looks good to you. To become a committer, you had better practice to press `Approve` button with `LGTM`. :)
--
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: dev-unsubscribe@orc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org