-
Notifications
You must be signed in to change notification settings - Fork 3k
Reduce code duplication in Arrow code #2746
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
ed4ca7e to
0addb2c
Compare
0addb2c to
0652940
Compare
|
@rymurr it is probably easier looking at the modified files directly instead of looking at the diff when reviewing. |
0652940 to
4ddd3fa
Compare
Results on branch
|
rymurr
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One minor comment @nastra . Looks great though, the vectorised code is a lot easier to read! I love deleting code!
| class DictionaryIdReader extends BaseDictEncodedReader { | ||
| @Override | ||
| protected void nextVal(FieldVector vector, Dictionary dict, int idx, int currentVal, int typeWidth) { | ||
| ((IntVector) vector).set(idx, currentVal); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason this is cast to IntVector and the others are directly manipulating the data buffer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dictionary encoded vectors are always represented as IntVector, but since BaseDictEncodedReader uses the more generic FieldVector we need to do a cast to IntVector here. Fwiw, here's how it was done in the original code:
Line 55 in 6bcca16
| intVector.set(idx, currentValue); |
iceberg/arrow/src/main/java/org/apache/iceberg/arrow/vectorized/VectorizedArrowReader.java
Line 136 in 6bcca16
| vectorizedColumnIterator.nextBatchDictionaryIds((IntVector) vec, nullabilityHolder); |
rymurr
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks awesome @nastra
arrow/src/main/java/org/apache/iceberg/arrow/vectorized/parquet/VectorizedColumnIterator.java
Outdated
Show resolved
Hide resolved
|
I'm fine either way. Whatever you'd like to do. |
5ebdd4c to
6c12d43
Compare
|
note - rebasing to keep each atomic class refactor as a separate (revertible) commit |
No description provided.