[SPARK-14015][SQL] Support TimestampType in vectorized parquet reader #11882
Conversation
|
cc @nongli |
|
Test build #53757 has finished for PR 11882 at commit
|
| case INT96: | ||
| if (column.dataType() == DataTypes.TimestampType) { | ||
| for (int i = rowId; i < rowId + num; ++i) { | ||
| Binary v = dictionary.decodeToBinary(dictionaryIds.getInt(i)); |
There was a problem hiding this comment.
hm -- maybe we can do something a lot cheaper? At the very least maybe we can remove the creation of the this Binary object, since we are turning it immediately into a Long.
There was a problem hiding this comment.
sure, that sounds good
There was a problem hiding this comment.
Added a TODO for converting the dictionary of binaries into long to make it cheaper
|
Test build #53759 has finished for PR 11882 at commit
|
| @@ -308,7 +322,7 @@ private void readIntBatch(int rowId, int num, ColumnVector column) throws IOExce | |||
|
|
|||
| private void readLongBatch(int rowId, int num, ColumnVector column) throws IOException { | |||
| // This is where we implement support for the valid type conversions. | |||
| if (column.dataType() == DataTypes.LongType || | |||
| if (column.dataType() == DataTypes.LongType || column.dataType() == DataTypes.TimestampType || | |||
| DecimalType.is64BitDecimalType(column.dataType())) { | |||
| defColumn.readLongs( | |||
There was a problem hiding this comment.
hmm. How does this work? readLongs is expecting to read parquet int64 physical types. How is it able to read this other physical type?
There was a problem hiding this comment.
Do we have tests where this type is not dictionary encoded?
There was a problem hiding this comment.
Thanks, fixed. Also added tests in HadoopFsRelationTest that test both the dictionary encoded and non-encoded versions across all supported datatypes.
|
Test build #53829 has finished for PR 11882 at commit
|
| @@ -259,8 +259,6 @@ private void initializeInternal() throws IOException { | |||
| if (!t.isPrimitive() || t.isRepetition(Type.Repetition.REPEATED)) { | |||
| throw new IOException("Complex types not supported."); | |||
| } | |||
| PrimitiveType primitiveType = t.asPrimitiveType(); | |||
|
|
|||
| originalTypes[i] = t.getOriginalType(); | |||
|
|
|||
| // TODO: Be extremely cautious in what is supported. Expand this. | |||
There was a problem hiding this comment.
I think we can remove this check now too.
There was a problem hiding this comment.
sure, sounds good.
|
LGTM |
|
Test build #53940 has finished for PR 11882 at commit
|
What changes were proposed in this pull request?
This PR adds support for TimestampType in the vectorized parquet reader
How was this patch tested?
VectorizedColumnReaderinitially had a gating condition onprimitiveType.getPrimitiveTypeName() == PrimitiveType.PrimitiveTypeName.INT96)that made us fall back on parquet-mr for handling timestamps. This condition is now removed.ParquetHadoopFsRelationSuite(that tests for all supported hive types -- includingTimestampType) fails when the gating condition is removed ([WIP][SPARK-13994][SQL] Investigate types not supported by vectorized parquet reader #11808) and should now pass with this change. Similarly, theParquetHiveCompatibilitySuite.SPARK-10177 timestamptest that fails when the gating condition is removed, should now pass as well.HadoopFsRelationTestthat test both the dictionary encoded and non-encoded versions across all supported datatypes.