You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@orc.apache.org by "cxzl25 (via GitHub)" <gi...@apache.org> on 2023/02/16 04:41:33 UTC
[GitHub] [orc] cxzl25 opened a new pull request, #1412: ORC-1373: Add log when DynamicByteArray length overflow
cxzl25 opened a new pull request, #1412:
URL: https://github.com/apache/orc/pull/1412
### What changes were proposed in this pull request?
When `DynamicByteArray` calculates `chunkIndex` overflow, it will throw NPE.
We can add a log to remind users to avoid this problem by configuring what ORC parameters.
### Why are the changes needed?
When the written string is very large, the grow calculation may overflow, causing the array data not to be expanded, and then NPE.
org.apache.orc.impl.DynamicByteArray#add(byte[], int, int)
```java
grow((length + valueLength) / chunkSize);
```
#### Log
```java
Caused by: java.lang.NullPointerException
at java.lang.System.arraycopy(Native Method)
at org.apache.orc.impl.DynamicByteArray.add(DynamicByteArray.java:115)
at org.apache.orc.impl.StringRedBlackTree.addNewKey(StringRedBlackTree.java:48)
at org.apache.orc.impl.StringRedBlackTree.add(StringRedBlackTree.java:60)
at org.apache.orc.impl.writer.StringTreeWriter.writeBatch(StringTreeWriter.java:69)
at org.apache.orc.impl.writer.StructTreeWriter.writeRootBatch(StructTreeWriter.java:56)
at org.apache.orc.impl.WriterImpl.addRowBatch(WriterImpl.java:696)
```
#### Local Test
org.apache.orc.impl.TestDynamicArray#testBigByteArray
```java
@Test
public void testBigByteArray() {
DynamicByteArray dba = new DynamicByteArray(128, 32 * 1024);
byte[] val = new byte[1024];
dba.add(val, 0, val.length);
byte[] bigVal = new byte[Integer.MAX_VALUE - 16];
dba.add(bigVal, 0, bigVal.length);
}
```
### How was this patch tested?
local test
Output>
```bash
2023-02-15 20:25:16,938 [main] ERROR DynamicByteArray: chunkIndex overflow:-65535. You can adjust the relevant configuration: orc.column.encoding.direct,orc.dictionary.key.threshold.
```
--
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] cxzl25 commented on pull request #1412: ORC-1373: Add log when DynamicByteArray length overflow
Posted by "cxzl25 (via GitHub)" <gi...@apache.org>.
cxzl25 commented on PR #1412:
URL: https://github.com/apache/orc/pull/1412#issuecomment-1434099073
> Could we throw a more accurate exception about it?
How about this?
```java
java.lang.RuntimeException: chunkIndex overflow:-65535. You can set orc.column.encoding.direct=columnName, or orc.dictionary.key.threshold=0 to turn off dictionary encoding.
at org.apache.orc.impl.DynamicByteArray.grow(DynamicByteArray.java:73)
at org.apache.orc.impl.DynamicByteArray.add(DynamicByteArray.java:122)
at org.apache.orc.impl.TestDynamicArray.testBigByteArray(TestDynamicArray.java:36)
```
--
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 #1412: ORC-1373: Add log when DynamicByteArray length overflow
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #1412:
URL: https://github.com/apache/orc/pull/1412#discussion_r1108054899
##########
java/core/src/java/org/apache/orc/impl/DynamicByteArray.java:
##########
@@ -59,10 +65,16 @@ private void grow(int chunkIndex) {
int newSize = Math.max(chunkIndex + 1, 2 * data.length);
data = Arrays.copyOf(data, newSize);
}
- for(int i=initializedChunks; i <= chunkIndex; ++i) {
+ for (int i = initializedChunks; i <= chunkIndex; ++i) {
data[i] = new byte[chunkSize];
}
initializedChunks = chunkIndex + 1;
+ } else if (chunkIndex < 0) {
+ LOG.error("chunkIndex overflow:{}. You can adjust the relevant configuration: {},{}.",
Review Comment:
Thank you for adding this. Can we give some directional hints how to adjust those configurations, @cxzl25 ?
--
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 #1412: ORC-1373: Provide directional exceptions instead of NPE in case of `DynamicByteArray` length overflow
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #1412:
URL: https://github.com/apache/orc/pull/1412#discussion_r1110399200
##########
java/core/src/java/org/apache/orc/impl/DynamicByteArray.java:
##########
@@ -59,10 +60,16 @@ private void grow(int chunkIndex) {
int newSize = Math.max(chunkIndex + 1, 2 * data.length);
data = Arrays.copyOf(data, newSize);
}
- for(int i=initializedChunks; i <= chunkIndex; ++i) {
+ for (int i = initializedChunks; i <= chunkIndex; ++i) {
data[i] = new byte[chunkSize];
}
initializedChunks = chunkIndex + 1;
+ } else if (chunkIndex < 0) {
Review Comment:
If NPE happens always before, this looks like a precondition check for this method.
- Shall we place this at the beginning of this method as an independent `if` statement?
- Could you add your new logic to the function description too? The current one looks vague.
https://github.com/apache/orc/blob/d403e11b7eab126259fb0683c0eb5c6131fd347b/java/core/src/java/org/apache/orc/impl/DynamicByteArray.java#L54
- Lastly, can we have a unit test to check the new exception message?
BTW, I revise the PR title, @cxzl25 . You can change back if you want.
--
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 #1412: ORC-1373: Add log when DynamicByteArray length overflow
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #1412:
URL: https://github.com/apache/orc/pull/1412#issuecomment-1434123301
+1 for @cxzl25 's new exception style because it was NPE already.
--
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 #1412: ORC-1373: Provide directional exceptions instead of NPE in case of `DynamicByteArray` length overflow
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #1412:
URL: https://github.com/apache/orc/pull/1412#issuecomment-1436103871
Also, cc @williamhyun
--
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] cxzl25 commented on a diff in pull request #1412: ORC-1373: Add log when DynamicByteArray length overflow
Posted by "cxzl25 (via GitHub)" <gi...@apache.org>.
cxzl25 commented on code in PR #1412:
URL: https://github.com/apache/orc/pull/1412#discussion_r1108110140
##########
java/core/src/java/org/apache/orc/impl/DynamicByteArray.java:
##########
@@ -59,10 +65,16 @@ private void grow(int chunkIndex) {
int newSize = Math.max(chunkIndex + 1, 2 * data.length);
data = Arrays.copyOf(data, newSize);
}
- for(int i=initializedChunks; i <= chunkIndex; ++i) {
+ for (int i = initializedChunks; i <= chunkIndex; ++i) {
data[i] = new byte[chunkSize];
}
initializedChunks = chunkIndex + 1;
+ } else if (chunkIndex < 0) {
+ LOG.error("chunkIndex overflow:{}. You can adjust the relevant configuration: {},{}.",
Review Comment:
Usually this problem occurs in the production environment, I usually set `orc.dictionary.key.threshold=0`.
Or find which field is a large string and skip it by `orc.column.encoding.direct=columnName`.
Because sometimes it is difficult to find which field is a large string, at this time, we can configure `orc.column.encoding.direct=*`. This is equivalent to `orc.dictionary.key.threshold=0`.
How about this?
```bash
2023-02-15 23:37:26,658 [main] ERROR DynamicByteArray: chunkIndex overflow:-65535. You can set orc.column.encoding.direct=columnName, or orc.dictionary.key.threshold=0 to turn off dictionary encoding.
```
--
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 #1412: ORC-1373: Provide directional exceptions instead of NPE in case of `DynamicByteArray` length overflow
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #1412:
URL: https://github.com/apache/orc/pull/1412#discussion_r1110392915
##########
java/core/src/java/org/apache/orc/impl/DynamicByteArray.java:
##########
@@ -59,10 +60,16 @@ private void grow(int chunkIndex) {
int newSize = Math.max(chunkIndex + 1, 2 * data.length);
data = Arrays.copyOf(data, newSize);
}
- for(int i=initializedChunks; i <= chunkIndex; ++i) {
+ for (int i = initializedChunks; i <= chunkIndex; ++i) {
data[i] = new byte[chunkSize];
}
initializedChunks = chunkIndex + 1;
+ } else if (chunkIndex < 0) {
Review Comment:
How many places do we have like the following?
```
grow((length + valueLength) / chunkSize);
```
--
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 #1412: ORC-1373: Provide directional exceptions instead of NPE in case of `DynamicByteArray` length overflow
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #1412:
URL: https://github.com/apache/orc/pull/1412#discussion_r1110399200
##########
java/core/src/java/org/apache/orc/impl/DynamicByteArray.java:
##########
@@ -59,10 +60,16 @@ private void grow(int chunkIndex) {
int newSize = Math.max(chunkIndex + 1, 2 * data.length);
data = Arrays.copyOf(data, newSize);
}
- for(int i=initializedChunks; i <= chunkIndex; ++i) {
+ for (int i = initializedChunks; i <= chunkIndex; ++i) {
data[i] = new byte[chunkSize];
}
initializedChunks = chunkIndex + 1;
+ } else if (chunkIndex < 0) {
Review Comment:
If NPE happens always before, this looks like a precondition check for this method.
- Shall we place this at the beginning of this method as an independent `if` statement?
- Could you add your new logic to the function description too? The current one looks vague.
https://github.com/apache/orc/blob/d403e11b7eab126259fb0683c0eb5c6131fd347b/java/core/src/java/org/apache/orc/impl/DynamicByteArray.java#L54
```
BTW, I revise the PR title, @cxzl25 . You can change back if you want.
--
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 #1412: ORC-1373: Provide directional exceptions instead of NPE in case of `DynamicByteArray` length overflow
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun closed pull request #1412: ORC-1373: Provide directional exceptions instead of NPE in case of `DynamicByteArray` length overflow
URL: https://github.com/apache/orc/pull/1412
--
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] deshanxiao commented on pull request #1412: ORC-1373: Add log when DynamicByteArray length overflow
Posted by "deshanxiao (via GitHub)" <gi...@apache.org>.
deshanxiao commented on PR #1412:
URL: https://github.com/apache/orc/pull/1412#issuecomment-1434025241
@cxzl25
Could we throw a more accurate exception about it?
--
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