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/07/08 14:21:15 UTC
[GitHub] [orc] belugabehr opened a new pull request #742: ORC-837: Reuse HiveDecimalWritable in ConvertTreeReaderFactory
belugabehr opened a new pull request #742:
URL: https://github.com/apache/orc/pull/742
### What changes were proposed in this pull request?
Create reusable HiveDecimalWritable objects instead of creating a new HiveDecimalWritable for each processed item.
### Why are the changes needed?
Performance, less garbage collection
### How was this patch tested?
No change in functionality. Use existing unit tests.
--
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 #742: ORC-837: Reuse HiveDecimalWritable in ConvertTreeReaderFactory
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun merged pull request #742:
URL: https://github.com/apache/orc/pull/742
--
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 #742: ORC-837: Reuse HiveDecimalWritable in ConvertTreeReaderFactory
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on a change in pull request #742:
URL: https://github.com/apache/orc/pull/742#discussion_r684720280
##########
File path: java/core/src/java/org/apache/orc/impl/ConvertTreeReaderFactory.java
##########
@@ -1560,11 +1566,12 @@ public void nextVector(ColumnVector previousVector,
local = TimeZone.getDefault();
useProlepticGregorian = context.useProlepticGregorian();
fileUsedProlepticGregorian = context.fileUsedProlepticGregorian();
+ this.reuse = new HiveDecimalWritable();
}
@Override
public void setConvertVectorElement(int elementNum) {
- Instant t = decimalToInstant(decimalColVector, elementNum);
+ Instant t = decimalToInstant(decimalColVector, elementNum, this.reuse);
Review comment:
Could you remove `this.` like the other variable in this 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: 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 #742: ORC-837: Reuse HiveDecimalWritable in ConvertTreeReaderFactory
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on a change in pull request #742:
URL: https://github.com/apache/orc/pull/742#discussion_r684719673
##########
File path: java/core/src/java/org/apache/orc/impl/ConvertTreeReaderFactory.java
##########
@@ -1239,18 +1241,21 @@ static Instant timestampToInstant(TimestampColumnVector vector, int element) {
* Convert a decimal to an Instant using seconds & nanos.
* @param vector the decimal64 column vector
* @param element the element number to use
+ * @param reuse the writable container to reuse
* @return the timestamp instant
*/
- static Instant decimalToInstant(DecimalColumnVector vector, int element) {
- // copy the value so that we can mutate it
- HiveDecimalWritable value = new HiveDecimalWritable(vector.vector[element]);
- long seconds = value.longValue();
+ static Instant decimalToInstant(DecimalColumnVector vector, int element, HiveDecimalWritable reuse) {
+ final HiveDecimalWritable writable = vector.vector[element];
Review comment:
Could you keep the original name, `value`, instead of this new name `writable`?
--
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] belugabehr commented on pull request #742: ORC-837: Reuse HiveDecimalWritable in ConvertTreeReaderFactory
Posted by GitBox <gi...@apache.org>.
belugabehr commented on pull request #742:
URL: https://github.com/apache/orc/pull/742#issuecomment-899761716
@dongjoon-hyun Thanks so much for all the attention this week. Should be good to go 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 commented on a change in pull request #742: ORC-837: Reuse HiveDecimalWritable in ConvertTreeReaderFactory
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on a change in pull request #742:
URL: https://github.com/apache/orc/pull/742#discussion_r684720251
##########
File path: java/core/src/java/org/apache/orc/impl/ConvertTreeReaderFactory.java
##########
@@ -1560,11 +1566,12 @@ public void nextVector(ColumnVector previousVector,
local = TimeZone.getDefault();
useProlepticGregorian = context.useProlepticGregorian();
fileUsedProlepticGregorian = context.fileUsedProlepticGregorian();
+ this.reuse = new HiveDecimalWritable();
Review comment:
Could you remove `this.` like the code around 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: 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 #742: ORC-837: Reuse HiveDecimalWritable in ConvertTreeReaderFactory
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on a change in pull request #742:
URL: https://github.com/apache/orc/pull/742#discussion_r684719242
##########
File path: java/core/src/java/org/apache/orc/impl/ConvertTreeReaderFactory.java
##########
@@ -814,21 +814,23 @@ public void nextVector(ColumnVector previousVector,
public static class DecimalFromAnyIntegerTreeReader extends ConvertTreeReader {
private LongColumnVector longColVector;
private ColumnVector decimalColVector;
+ private final HiveDecimalWritable reuse;
Review comment:
Shall we rename `reuse` to `value`?
--
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] belugabehr commented on pull request #742: ORC-837: Reuse HiveDecimalWritable in ConvertTreeReaderFactory
Posted by GitBox <gi...@apache.org>.
belugabehr commented on pull request #742:
URL: https://github.com/apache/orc/pull/742#issuecomment-899650633
@dongjoon-hyun I chose 'reuse' based on my experience with the Apache Avro project, but I can use whatever you'd like.
https://github.com/apache/avro/blob/42822886c28ea74a744abb7e7a80a942c540faa5/lang/java/avro/src/main/java/org/apache/avro/message/MessageDecoder.java#L56
--
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 #742: ORC-837: Reuse HiveDecimalWritable in ConvertTreeReaderFactory
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on a change in pull request #742:
URL: https://github.com/apache/orc/pull/742#discussion_r684720326
##########
File path: java/core/src/java/org/apache/orc/impl/ConvertTreeReaderFactory.java
##########
@@ -1548,6 +1553,7 @@ public void nextVector(ColumnVector previousVector,
private final TimeZone local;
private final boolean useProlepticGregorian;
private final boolean fileUsedProlepticGregorian;
+ private final HiveDecimalWritable reuse;
Review comment:
Shall we use `value` instead of `reuse`?
--
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 #742: ORC-837: Reuse HiveDecimalWritable in ConvertTreeReaderFactory
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on pull request #742:
URL: https://github.com/apache/orc/pull/742#issuecomment-899721735
Could you fix the checkstyle issue?
```
Error: src/java/org/apache/orc/impl/ConvertTreeReaderFactory.java:[1258] (sizes) LineLength: Line is longer than 100 characters (found 103).
```
--
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 #742: ORC-837: Reuse HiveDecimalWritable in ConvertTreeReaderFactory
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on a change in pull request #742:
URL: https://github.com/apache/orc/pull/742#discussion_r684720035
##########
File path: java/core/src/java/org/apache/orc/impl/ConvertTreeReaderFactory.java
##########
@@ -1239,18 +1241,21 @@ static Instant timestampToInstant(TimestampColumnVector vector, int element) {
* Convert a decimal to an Instant using seconds & nanos.
* @param vector the decimal64 column vector
* @param element the element number to use
+ * @param reuse the writable container to reuse
* @return the timestamp instant
*/
- static Instant decimalToInstant(DecimalColumnVector vector, int element) {
- // copy the value so that we can mutate it
- HiveDecimalWritable value = new HiveDecimalWritable(vector.vector[element]);
- long seconds = value.longValue();
+ static Instant decimalToInstant(DecimalColumnVector vector, int element, HiveDecimalWritable reuse) {
Review comment:
For this new parameter, `HiveDecimalWritable reuse`, I'm not sure about the proper name.
--
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