wypoon commented on code in PR #11661:
URL: https://github.com/apache/iceberg/pull/11661#discussion_r2118184966
##########
arrow/src/main/java/org/apache/iceberg/arrow/vectorized/parquet/VectorizedParquetDefinitionLevelReader.java:
##########
@@ -45,122 +46,217 @@ public VectorizedParquetDefinitionLevelReader(
super(bitWidth, maxDefLevel, readLength, setArrowValidityVector);
}
- abstract class NumericBaseReader {
- public void nextBatch(
+ abstract class CommonReader {
+ private void nextBatch(
final FieldVector vector,
final int startOffset,
final int typeWidth,
final int numValsToRead,
NullabilityHolder nullabilityHolder,
- ValuesAsBytesReader valuesReader) {
- int bufferIdx = startOffset;
+ ValuesReader valuesReader,
+ Dictionary dict) {
+ int idx = startOffset;
int left = numValsToRead;
while (left > 0) {
if (currentCount == 0) {
readNextGroup();
}
int numValues = Math.min(left, currentCount);
+
+ byte[] byteArray = null;
+ if (typeWidth > -1) {
+ byteArray = new byte[typeWidth];
+ }
+ ArrowBuf validityBuffer = vector.getValidityBuffer();
+
switch (mode) {
case RLE:
- setNextNValuesInVector(
- typeWidth, nullabilityHolder, valuesReader, bufferIdx, vector,
numValues);
- bufferIdx += numValues;
+ if (valuesReader instanceof ValuesAsBytesReader) {
+ nextRleBatch(
+ vector,
+ typeWidth,
+ nullabilityHolder,
+ (ValuesAsBytesReader) valuesReader,
+ idx,
+ numValues,
+ byteArray);
+ } else if (valuesReader instanceof
VectorizedDictionaryEncodedParquetValuesReader) {
+ nextRleDictEncodedBatch(
+ vector,
+ typeWidth,
+ nullabilityHolder,
+ (VectorizedDictionaryEncodedParquetValuesReader)
valuesReader,
+ dict,
+ idx,
+ numValues,
+ validityBuffer);
+ }
+ idx += numValues;
break;
case PACKED:
- for (int i = 0; i < numValues; ++i) {
- if (packedValuesBuffer[packedValuesBufferIdx++] == maxDefLevel) {
- nextVal(vector, bufferIdx * typeWidth, valuesReader, mode);
- nullabilityHolder.setNotNull(bufferIdx);
- if (setArrowValidityVector) {
- BitVectorHelper.setBit(vector.getValidityBuffer(),
bufferIdx);
- }
- } else {
- setNull(nullabilityHolder, bufferIdx,
vector.getValidityBuffer());
- }
- bufferIdx++;
+ if (valuesReader instanceof ValuesAsBytesReader) {
Review Comment:
@pvary thank you for the suggestion. I have implemented it after some
consideration. At first, I had reservations because the original refactor
seemed relatively easy to compare to the pre-refactor code. It was not too
difficult to compare the 4 methods pre-refactor that were structurally similar
to the common method in the refactor and verify that the logic is preserved.
With your suggested change, the refactor seems more radical. However, it does
seem elegant and avoids the use of `instanceof` (which I suppose you still have
reservations about! ;-)).
If you look at the 3 commits currently in this PR, it is easy to see that
the 2nd one is some simple renaming, and the 3rd one -- the change to adopt
your suggestion -- is quite easy to verify does not change the logic from the
2nd; so the PR is not so difficult to verify that way.
--
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-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
