Skip to content

ORC-1384: Fix ArrayIndexOutOfBoundsException when reading dictionary stream bigger then dictionary - #1431

Closed
zratkai wants to merge 2 commits into
apache:mainfrom
zratkai:ORC-1384
Closed

ORC-1384: Fix ArrayIndexOutOfBoundsException when reading dictionary stream bigger then dictionary#1431
zratkai wants to merge 2 commits into
apache:mainfrom
zratkai:ORC-1384

Conversation

@zratkai

@zratkaizratkai commented Mar 7, 2023•
edited by guiyanakuang
Loading

Copy link
Copy Markdown
Contributor

What changes were proposed in this pull request?

Avoid ArrayIndexOutOfBoundsException when reading dictionary stream bigger then dictionary. Check the size of the dictionary and input and read only the min of those.

Why are the changes needed?

In Hive when reading with LLAP data is read in 4kB blocks which leads to ArrayIndexOutOfBoundsException when the dictionary is smaller.

How was this patch tested?

It is tested with HIVE's qtest, since here we do not have the necessary subclasses.

@guiyanakuangguiyanakuang changed the title ORC-1384 Fix ArrayIndexOutOfBoundsException when reading dictionary s…ORC-1384 Fix ArrayIndexOutOfBoundsException when reading dictionary stream bigger then dictionaryMar 8, 2023
guiyanakuang
guiyanakuang reviewed Mar 8, 2023
View reviewed changes
Comment threadjava/core/src/java/org/apache/orc/impl/TreeReaderFactory.java
@deshanxiao

deshanxiao commented Mar 8, 2023•
edited
Loading

Copy link
Copy Markdown
Contributor

Thank you for making the PR.
Could you make the CI happy with mvn checkstyle:check -Panalyze -Pbenchmark ? @zratkai

[INFO] There is 1 error reported by Checkstyle 9.3 with checkstyle.xml ruleset.
Error: src/java/org/apache/orc/impl/TreeReaderFactory.java:[2295] (sizes) LineLength: Line is longer than 100 characters (found 114).

guiyanakuang
guiyanakuang reviewed Mar 8, 2023
View reviewed changes
Comment threadjava/core/src/java/org/apache/orc/impl/TreeReaderFactory.java
Comment threadjava/core/src/java/org/apache/orc/impl/TreeReaderFactory.java
@deshanxiao

deshanxiao commented Mar 8, 2023

Copy link
Copy Markdown
Contributor

Actually, the issue blocked the upgrade of ORC in Hive:
apache/hive#3833

@zratkai

zratkai commented Mar 8, 2023

Copy link
Copy Markdown
ContributorAuthor

@deshanxiao thanks for adding this info. Yes, exactly, this was the original issue I started with. To finish it this fix is needed.

@zratkai
ORC-1384 Fix ArrayIndexOutOfBoundsException when reading dictionary s…
8ea77fd
…tream bigger then dictionary
### What changes were proposed in this pull request?
Avoid ArrayIndexOutOfBoundsException when reading dictionary stream bigger then dictionary. Check the size of the dictionary and input and read only the min of those.
### Why are the changes needed?
In Hive when reading with LLAP data is read in 4kB blocks which leads to ArrayIndexOutOfBoundsException when the dictionary is smaller.
### How was this patch tested?
It is tested with HIVE's qtest, since here we do not have the necessary subclasses.
This closesapache#1384
@dongjoon-hyundongjoon-hyun changed the title ORC-1384 Fix ArrayIndexOutOfBoundsException when reading dictionary stream bigger then dictionaryORC-1384: Fix ArrayIndexOutOfBoundsException when reading dictionary stream bigger then dictionaryMar 8, 2023
dongjoon-hyun
dongjoon-hyun reviewed Mar 8, 2023
View reviewed changes

@dongjoon-hyundongjoon-hyun left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for making a PR, @zratkai .

  • Do you happen to know what caused this regression?
  • Do you think you can add a small unit test which you described in the commets?

@dongjoon-hyun

dongjoon-hyun commented Mar 8, 2023

Copy link
Copy Markdown
Member

Also cc @williamhyun

@dongjoon-hyundongjoon-hyun added this to the 1.8.3 milestone Mar 8, 2023
@zratkai

zratkai commented Mar 9, 2023

Copy link
Copy Markdown
ContributorAuthor

@dongjoon-hyun

Thank you for making a PR, @zratkai .

  • Do you happen to know what caused this regression?
  • Do you think you can add a small unit test which you described in the commets?
  1. It is caused by LLAP in Hive, since it reads in 4kB blocks as I described above.
  2. I was thinking about that, but it is not easy for this modification, since it is in a private method, and we don't have any test even for the public method which calls this (org.apache.orc.impl.TreeReaderFactory.StringDictionaryTreeReader#nextVector) . So it would mean I need to create for that as well from scratch.

@dongjoon-hyundongjoon-hyun changed the title ORC-1384: Fix ArrayIndexOutOfBoundsException when reading dictionary stream bigger then dictionaryORC-1384: Fix ArrayIndexOutOfBoundsException when reading dictionary stream bigger then dictionaryMar 9, 2023
dongjoon-hyun
dongjoon-hyun approved these changes Mar 9, 2023
View reviewed changes

@dongjoon-hyundongjoon-hyun left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1, LGTM.
Merged to main/1.8 for Apache ORC 1.8.3

dongjoon-hyun added a commit that referenced this pull request Mar 9, 2023
@zratkai@dongjoon-hyun
ORC-1384: FixArrayIndexOutOfBoundsExceptionwhen reading dictionar…
5576f18
…y stream bigger then dictionary
### What changes were proposed in this pull request?
Avoid ArrayIndexOutOfBoundsException when reading dictionary stream bigger then dictionary. Check the size of the dictionary and input and read only the min of those.
### Why are the changes needed?
In Hive when reading with LLAP data is read in 4kB blocks which leads to ArrayIndexOutOfBoundsException when the dictionary is smaller.
### How was this patch tested?
It is tested with HIVE's qtest, since here we do not have the necessary subclasses.
Closes#1431 from zratkai/ORC-1384.
Lead-authored-by: Zoltan Ratkai <zratkai@cloudera.com>
Co-authored-by: Dongjoon Hyun <dongjoon@apache.org>
Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
(cherry picked from commit 8cf9057)
Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
@dongjoon-hyun

dongjoon-hyun commented Mar 9, 2023

Copy link
Copy Markdown
Member

Thank you, @zratkai and @guiyanakuang .

Also, cc @williamhyun

@dongjoon-hyun

dongjoon-hyun commented Mar 9, 2023

Copy link
Copy Markdown
Member

BTW, welcome to the Apache ORC community, @zratkai .
ORC-1384 is resolved and assigned to you.

@zratkaizratkai mentioned this pull request Apr 3, 2023
Merged
cxzl25 pushed a commit to cxzl25/orc that referenced this pull request Jan 11, 2024
@zratkai@dongjoon-hyun@cxzl25
ORC-1384: Fix ArrayIndexOutOfBoundsException when reading dictionar…
942b72d
…y stream bigger then dictionary
### What changes were proposed in this pull request?
Avoid ArrayIndexOutOfBoundsException when reading dictionary stream bigger then dictionary. Check the size of the dictionary and input and read only the min of those.
### Why are the changes needed?
In Hive when reading with LLAP data is read in 4kB blocks which leads to ArrayIndexOutOfBoundsException when the dictionary is smaller.
### How was this patch tested?
It is tested with HIVE's qtest, since here we do not have the necessary subclasses.
Closesapache#1431 from zratkai/ORC-1384.
Lead-authored-by: Zoltan Ratkai <zratkai@cloudera.com>
Co-authored-by: Dongjoon Hyun <dongjoon@apache.org>
Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@guiyanakuangguiyanakuangguiyanakuang left review comments
@deshanxiaodeshanxiaodeshanxiao left review comments
@dongjoon-hyundongjoon-hyundongjoon-hyun approved these changes

Assignees

No one assigned

Labels

JAVA

Projects

None yet

Milestone

1.8.3

Development

Successfully merging this pull request may close these issues.

4 participants

@zratkai@deshanxiao@dongjoon-hyun@guiyanakuang