singhpk234 commented on code in PR #3249:
URL: https://github.com/apache/iceberg/pull/3249#discussion_r861231678
##########
arrow/src/main/java/org/apache/iceberg/arrow/vectorized/VectorHolder.java:
##########
@@ -51,6 +52,25 @@ public VectorHolder(
this.dictionary = dictionary;
this.nullabilityHolder = holder;
this.icebergType = type;
+ this.originalIcebergType = type;
+ }
+
+ public VectorHolder(
Review Comment:
+1, if we want to keep it we can move the preconditions / assignments to the
new constructor and just call :
```java
this(columnDescriptor, vector, isDictionaryEncoded, dictionary, holder,
type, type);
```
from old constructor.
##########
arrow/src/main/java/org/apache/iceberg/arrow/vectorized/GenericArrowVectorAccessorFactory.java:
##########
@@ -163,12 +165,18 @@ private ArrowVectorAccessor<DecimalT, Utf8StringT,
ArrayT, ChildVectorT> getDict
@SuppressWarnings("checkstyle:CyclomaticComplexity")
private ArrowVectorAccessor<DecimalT, Utf8StringT, ArrayT, ChildVectorT>
- getPlainVectorAccessor(FieldVector vector) {
+ getPlainVectorAccessor(FieldVector vector, PrimitiveType primitive) {
if (vector instanceof BitVector) {
return new BooleanAccessor<>((BitVector) vector);
} else if (vector instanceof IntVector) {
+ if (primitive != null &&
OriginalType.DECIMAL.equals(primitive.getOriginalType())) {
Review Comment:
[minor] This logic of using can be extracted in local variable in start of
function , subsequently we can check that local in L#172, L#177, L#206
##########
arrow/src/main/java/org/apache/iceberg/arrow/vectorized/VectorizedArrowReader.java:
##########
@@ -84,13 +85,28 @@ public VectorizedArrowReader(
BufferAllocator ra,
boolean setArrowValidityVector) {
this.icebergField = icebergField;
+ this.logicalTypeField = icebergField;
+ this.columnDescriptor = desc;
+ this.rootAlloc = ra;
+ this.vectorizedColumnIterator = new VectorizedColumnIterator(desc, "",
setArrowValidityVector);
Review Comment:
can call new constructor with fields we don't have as null / empty.
##########
arrow/src/main/java/org/apache/iceberg/arrow/vectorized/GenericArrowVectorAccessorFactory.java:
##########
@@ -92,12 +93,13 @@ public ArrowVectorAccessor<DecimalT, Utf8StringT, ArrayT,
ChildVectorT> getVecto
Dictionary dictionary = holder.dictionary();
boolean isVectorDictEncoded = holder.isDictionaryEncoded();
FieldVector vector = holder.vector();
+ ColumnDescriptor desc = holder.descriptor();
+ // desc could be null when the holder is PositionVectorHolder
Review Comment:
[question]
[ConstantVectorHolder](https://github.com/apache/iceberg/blob/master/arrow/src/main/java/org/apache/iceberg/arrow/vectorized/VectorHolder.java#L119-L141)
can also have descriptor() value as null, is it not being called out here as
its mostly treated as `dummy` ?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
