You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "wgtmac (via GitHub)" <gi...@apache.org> on 2024/02/22 06:22:11 UTC
[PR] Parquet, Arrow: Refactor vectorized reader [iceberg]
wgtmac opened a new pull request, #9772:
URL: https://github.com/apache/iceberg/pull/9772
This patch is the preparation work of https://github.com/apache/iceberg/issues/7162 and it contains following changes:
- Add VectorizedValuesReader interface for future extension.
- Refactor VectorizedPageIterator to use VectorizedValuesReader.
- Consolidate duplicate code in the VectorizedPageIterator.
--
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@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Parquet, Arrow: Refactor vectorized reader [iceberg]
Posted by "anthonysgro (via GitHub)" <gi...@apache.org>.
anthonysgro commented on PR #9772:
URL: https://github.com/apache/iceberg/pull/9772#issuecomment-1989273962
@wgtmac thanks so much for taking this issue on! I am just a user of iceberg (I have never contributed), but I was curious--is it also possible to include a resolution for https://github.com/apache/iceberg/issues/521 as an extension of this work? It would be a huge boon for iceberg performance since I often use complex/nested types, and I'm sure others do as well.
--
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@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Parquet, Arrow: Refactor vectorized reader [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on PR #9772:
URL: https://github.com/apache/iceberg/pull/9772#issuecomment-1999811652
> I plan to resolve https://github.com/apache/iceberg/issues/7162 by adding vectorized readers for all v2 encodings. This is the 1st patch.
@wgtmac do you by any chance have all other changes that are required for v2 encodings? I'd like to see and understand how the changes in this PR fit into the bigger picture (It's been a while since I touched/reviewed the vectorized code paths)
--
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@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Parquet, Arrow: Refactor vectorized reader [iceberg]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on PR #9772:
URL: https://github.com/apache/iceberg/pull/9772#issuecomment-2641355085
Hi @wgtmac , can we resume this PR? I can help with review.
--
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@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Parquet, Arrow: Refactor vectorized reader [iceberg]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on PR #9772:
URL: https://github.com/apache/iceberg/pull/9772#issuecomment-2248230178
Since this feature has been requested in the community but the PR is not reviewed for a long time. Do you think this is the right direction here? Or should I directly support reading arrow vectors from the parquet-java library? @nastra @aokolnychyi @Fokko
--
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@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Parquet, Arrow: Refactor vectorized reader [iceberg]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #9772:
URL: https://github.com/apache/iceberg/pull/9772#issuecomment-2425314026
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions.
--
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@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Parquet, Arrow: Refactor vectorized reader [iceberg]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #9772:
URL: https://github.com/apache/iceberg/pull/9772#discussion_r1527197363
##########
arrow/src/main/java/org/apache/iceberg/arrow/vectorized/parquet/VectorizedParquetDefinitionLevelReader.java:
##########
@@ -47,35 +46,45 @@ public VectorizedParquetDefinitionLevelReader(
super(bitWidth, maxDefLevel, readLength, setArrowValidityVector);
}
- abstract class NumericBaseReader {
+ abstract class BaseReader {
Review Comment:
I have merged `NumericBaseReader` into `BaseReader` to eliminate the duplicate code.
##########
arrow/src/main/java/org/apache/iceberg/arrow/vectorized/parquet/VectorizedPageIterator.java:
##########
@@ -95,7 +94,9 @@ protected void initDataReader(Encoding dataEncoding, ByteBufferInputStream in, i
throw new ParquetDecodingException("could not read page in col " + desc, e);
}
} else {
- if (dataEncoding != Encoding.PLAIN) {
+ if (dataEncoding == Encoding.PLAIN) {
+ valuesReader = new VectorizedPlainValuesReader();
+ } else {
Review Comment:
More VectorizedValuesReader implementations will be added here for different encodings.
##########
arrow/src/main/java/org/apache/iceberg/arrow/vectorized/parquet/VectorizedValuesReader.java:
##########
@@ -0,0 +1,55 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.arrow.vectorized.parquet;
+
+import org.apache.arrow.vector.FieldVector;
+import org.apache.parquet.bytes.ByteBufferInputStream;
+import org.apache.parquet.io.api.Binary;
+
+/** Interface for value decoding that supports vectorized (aka batched) decoding. */
+interface VectorizedValuesReader {
Review Comment:
This is the new package-private vectorized reader for a single page. All v2 encodings will be added by implementing this interface and selected readXXX() methods.
##########
arrow/src/main/java/org/apache/iceberg/arrow/vectorized/parquet/VectorizedPageIterator.java:
##########
@@ -46,7 +45,7 @@ public VectorizedPageIterator(
this.setArrowValidityVector = setValidityVector;
}
- private ValuesAsBytesReader plainValuesReader = null;
+ private VectorizedValuesReader valuesReader = null;
Review Comment:
I have replaced plainValuesReader with generic values reader to support different encodings.
##########
arrow/src/main/java/org/apache/iceberg/arrow/vectorized/parquet/VectorizedParquetDefinitionLevelReader.java:
##########
@@ -150,280 +159,195 @@ public void nextDictEncodedBatch(
}
protected abstract void nextVal(
- FieldVector vector, int idx, ValuesAsBytesReader valuesReader, Mode mode);
+ FieldVector vector, int idx, VectorizedValuesReader valuesReader, int typeWidth);
protected abstract void nextDictEncodedVal(
FieldVector vector,
int idx,
- VectorizedDictionaryEncodedParquetValuesReader dictionaryEncodedValuesReader,
+ VectorizedDictionaryEncodedParquetValuesReader reader,
+ int numValuesToRead,
Dictionary dict,
- Mode mode,
+ NullabilityHolder nullabilityHolder,
+ int typeWidth,
+ Mode mode);
+
+ protected void nextValues(
Review Comment:
This method is where we utilize the vectorized reading method from VectorizedValuesReader. You can see LongReader/IntegerReader/FloatReader/DoubleReader have overridden this method.
##########
arrow/src/main/java/org/apache/iceberg/arrow/vectorized/parquet/VectorizedParquetDefinitionLevelReader.java:
##########
@@ -150,280 +159,195 @@ public void nextDictEncodedBatch(
}
protected abstract void nextVal(
- FieldVector vector, int idx, ValuesAsBytesReader valuesReader, Mode mode);
+ FieldVector vector, int idx, VectorizedValuesReader valuesReader, int typeWidth);
protected abstract void nextDictEncodedVal(
FieldVector vector,
int idx,
- VectorizedDictionaryEncodedParquetValuesReader dictionaryEncodedValuesReader,
+ VectorizedDictionaryEncodedParquetValuesReader reader,
+ int numValuesToRead,
Dictionary dict,
- Mode mode,
+ NullabilityHolder nullabilityHolder,
+ int typeWidth,
+ Mode mode);
+
+ protected void nextValues(
+ FieldVector vector,
+ int bufferIdx,
int numValues,
- NullabilityHolder holder,
- int typeWidth);
+ VectorizedValuesReader valuesReader,
+ int typeWidth) {
+ for (int i = 0; i < numValues; i++) {
+ nextVal(vector, bufferIdx + i, valuesReader, typeWidth);
+ }
+ }
+
+ // Used only for fixed-length binary types
+ @SuppressWarnings("checkstyle:VisibilityModifier")
+ protected byte[] byteArray;
}
- class LongReader extends NumericBaseReader {
+ class LongReader extends BaseReader {
@Override
protected void nextVal(
- FieldVector vector, int idx, ValuesAsBytesReader valuesReader, Mode mode) {
- vector.getDataBuffer().setLong(idx, valuesReader.readLong());
+ FieldVector vector, int idx, VectorizedValuesReader valuesReader, int typeWidth) {
+ vector.getDataBuffer().setLong((long) idx * typeWidth, valuesReader.readLong());
}
@Override
protected void nextDictEncodedVal(
FieldVector vector,
int idx,
- VectorizedDictionaryEncodedParquetValuesReader dictionaryEncodedValuesReader,
+ VectorizedDictionaryEncodedParquetValuesReader reader,
+ int numValuesToRead,
Dictionary dict,
- Mode mode,
- int numValues,
- NullabilityHolder holder,
- int typeWidth) {
+ NullabilityHolder nullabilityHolder,
+ int typeWidth,
+ Mode mode) {
if (Mode.RLE.equals(mode)) {
- dictionaryEncodedValuesReader
+ reader
.longDictEncodedReader()
- .nextBatch(vector, idx, numValues, dict, holder, typeWidth);
+ .nextBatch(vector, idx, numValuesToRead, dict, nullabilityHolder, typeWidth);
} else if (Mode.PACKED.equals(mode)) {
vector
.getDataBuffer()
- .setLong(
- (long) idx * typeWidth,
- dict.decodeToLong(dictionaryEncodedValuesReader.readInteger()));
+ .setLong((long) idx * typeWidth, dict.decodeToLong(reader.readInteger()));
}
}
+
+ @Override
+ protected void nextValues(
+ FieldVector vector,
+ int bufferIdx,
+ int numValues,
+ VectorizedValuesReader valuesReader,
+ int typeWidth) {
+ valuesReader.readLongs(numValues, vector, bufferIdx);
Review Comment:
For example, `LongReader` overrides nextValues to use `VectorizedValuesReader.readLongs()` method for vectorized reading.
--
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@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Parquet, Arrow: Refactor vectorized reader [iceberg]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on PR #9772:
URL: https://github.com/apache/iceberg/pull/9772#issuecomment-1959648527
I plan to resolve https://github.com/apache/iceberg/issues/7162 by adding vectorized readers for all v2 encodings. This is the 1st patch. Would you mind taking a look? @rdblue @nastra @Fokko @jackye1995
--
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@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Parquet, Arrow: Refactor vectorized reader [iceberg]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #9772:
URL: https://github.com/apache/iceberg/pull/9772#discussion_r1527197363
##########
arrow/src/main/java/org/apache/iceberg/arrow/vectorized/parquet/VectorizedParquetDefinitionLevelReader.java:
##########
@@ -47,35 +46,45 @@ public VectorizedParquetDefinitionLevelReader(
super(bitWidth, maxDefLevel, readLength, setArrowValidityVector);
}
- abstract class NumericBaseReader {
+ abstract class BaseReader {
Review Comment:
I have merged `NumericBaseReader` into `BaseReader` to eliminate the duplicate code. This refactoring contributes to the most part of this PR.
--
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@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Parquet, Arrow: Refactor vectorized reader [iceberg]
Posted by "PaulLiang1 (via GitHub)" <gi...@apache.org>.
PaulLiang1 commented on PR #9772:
URL: https://github.com/apache/iceberg/pull/9772#issuecomment-2425321714
Bump comment to keep the bot from closing this
--
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@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Parquet, Arrow: Refactor vectorized reader [iceberg]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on PR #9772:
URL: https://github.com/apache/iceberg/pull/9772#issuecomment-1989767289
@anthonysgro Yes, that is on my radar. I will work on it once v2 encodings of all primitive types have been supported.
--
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@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Parquet, Arrow: Refactor vectorized reader [iceberg]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on PR #9772:
URL: https://github.com/apache/iceberg/pull/9772#issuecomment-2047925627
@nastra Gentle ping.
--
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@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Parquet, Arrow: Refactor vectorized reader [iceberg]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #9772:
URL: https://github.com/apache/iceberg/pull/9772#discussion_r1527198459
##########
arrow/src/main/java/org/apache/iceberg/arrow/vectorized/parquet/VectorizedParquetDefinitionLevelReader.java:
##########
@@ -47,35 +46,45 @@ public VectorizedParquetDefinitionLevelReader(
super(bitWidth, maxDefLevel, readLength, setArrowValidityVector);
}
- abstract class NumericBaseReader {
+ abstract class BaseReader {
Review Comment:
Perhaps I can split the refactoring work into a separate PR to make the review much easier.
--
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@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Parquet, Arrow: Refactor vectorized reader [iceberg]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on PR #9772:
URL: https://github.com/apache/iceberg/pull/9772#issuecomment-1999964002
Not yet. My rough plan is to do following things:
1. add a new VectorizedValuesReader base class to supporting different encodings. This is similar to what spark does but reading into arrow field vector: :https://github.com/apache/spark/blob/b7aa9740249b50ad9db254626c530ff5bc33d385/sql/core/src/main/java/org/apache/spark/sql/execution/datasources/parquet/VectorizedValuesReader.java#L30
2. extend VectorizedValuesReader to add v2 encodings one by one.
3. support vectorized readers for nested types.
This patch is the step 1 above and only added vectorized reading interfaces for float/double/int32/int64 physical types. It already shows the big picture that how following steps will be done. More interfaces will be added for other physical types and logical types into arrow field vectors will be added progressively for better review experience.
Does this make sense to you? @nastra
--
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@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Parquet, Arrow: Refactor vectorized reader [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on PR #9772:
URL: https://github.com/apache/iceberg/pull/9772#issuecomment-2248258706
hey @wgtmac sorry for the long wait here but I don't have any spare cycles atm to review this.
--
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@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Parquet, Arrow: Refactor vectorized reader [iceberg]
Posted by "wgtmac (via GitHub) via issues" <is...@iceberg.apache.org>.
wgtmac closed pull request #9772: Parquet, Arrow: Refactor vectorized reader
URL: https://github.com/apache/iceberg/pull/9772
--
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@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org