Further refactor Parquet readers for v2 support#13290
Conversation
| @Override | ||
| protected void initDataReader(Encoding dataEncoding, ByteBufferInputStream in, int valueCount) { | ||
| ValuesReader previousReader = plainValuesReader; | ||
| ValuesReader previousReader = (ValuesReader) valuesReader; |
There was a problem hiding this comment.
Yeah currently we need the cast due to a bit of a Java-based pickle, happy to change this up if you have any ideas on how it can be improved.
VectorizedValuesReaderis currently not bound toValuesReader. We can't just makeVectorizedValuesReaderextendValuesReaderbecause the former is an interface and the latter is a class.- We can't just make
VectorizedValuesReaderan ABC because then classes likeVectorizedPlainValuesReadercan't extend both it andPlainValuesReader. - Since
ValuesReaderis an ABC in Parquet butVectorizedValuesReaderis an interface, we can't easily markvaluesReaderas being both types.- We could create a generic method and bind the type to
<T extends ValuesReader & VectorizedValuesReader>but that feels clunky
- We could create a generic method and bind the type to
I see a couple of ways out of this but they mostly involve copying a lot of code and I thought this cast wasn't too high of a price to pay. It does mean that we need to more or less keep VectorizedValuesReader in sync with ValuesReader if it evolves.
(This cast is inherited from #9772)
| import org.apache.parquet.io.api.Binary; | ||
|
|
||
| /** Interface for value decoding that supports vectorized (aka batched) decoding. */ | ||
| interface VectorizedValuesReader { |
There was a problem hiding this comment.
In the Spark corresponding class there are some other methods available, do we need those as well? Part of a followup or un-needed?
There was a problem hiding this comment.
A Spark expert can correct me, but I think the read methods with a WritableColumnVector parameter are for copying the data into a Spark WritableColumnVector. In Iceberg we use Arrow vectors instead of WritableColumnVectors. The skip methods are for Parquet page skipping. For the v2 read support, they are not needed. I'd like to resurrect my work on Parquet page skipping, so I may look into leveraging those skip methods myself, but for now, we don't need them.
There was a problem hiding this comment.
Yeah that's exactly correct. I played with the idea of implementing something like WritableColumnVector within Iceberg (but for building something like a VectorizedColumnIterator), but I think we won't need all of that code after all.
There was a problem hiding this comment.
Thanks @wypoon and @eric-maynard, that all sounds good to me
|
I have some small questions about the Roadmap for where we go from here but this makes sense to me as a first step. As long as we are more or less copying the Spark approach I think we are probably safe here. @huaxingao Could you do a quick check as well? |
|
Some weird rebase happened here, git history looks scary now :) |
a63ee5d to
9ecc2be
Compare
|
@eric-maynard Thanks for the PR! The approach looks good to me and seems like a reasonable first step. |
|
Thanks @huaxingao! I've added Javadocs About the scary diff @RussellSpitzer, it should be fixed but unfortunately I can't remove the tags which got auto-added when the diff was artificially massive |
|
@huaxingao and @wypoon do y'all have any other comment on this pr? |
|
Merged, Thanks @eric-maynard for the pr, and @huaxingao and @wypoon for reviewing. |
|
Also thanks @wgtmac for starting this! |
|
|
||
| class VectorizedPlainValuesReader extends ValuesAsBytesReader implements VectorizedValuesReader { | ||
|
|
||
| public static final int INT_SIZE = 4; |
There was a problem hiding this comment.
do these need to be public? They don't seem to be used anywhere outside of this class
There was a problem hiding this comment.
They’re used in the follow up PR (though currently they could be protected), but they could have been private in this PR. Good catch
In issues like #7162 and #11371, it's reported that newer Parquet encodings like DELTA_BINARY_PACKED don't work with the current Parquet readers. #11661 recently refactored the Parquet readers to improve code re-use, but there a few more changes needed to prepare us for Parquet v2 support.
This refactor introduces a new interface
VectorizedValuesReaderand changes readers likeTimestampMillisReaderto work with this new type. After this change, new implementations ofVectorizedValuesReadercan be added to support encodings like DELTA_BINARY_PACKED.This PR is a revival of @wgtmac's #9772, which based on our conversion he will not be able to continue work on. Thanks for the great work, @wgtmac.