Support for dictionary encoded INT96 timestamp in parquet files #4680

rui-mo · 2023-04-20T07:17:43Z

Support timestamp reader for Parquet file format to read from INT96 timestamps.

netlify · 2023-04-20T07:17:57Z

✅ Deploy Preview for meta-velox canceled.

Name	Link
🔨 Latest commit	`8a3df69`
🔍 Latest deploy log	https://app.netlify.com/sites/meta-velox/deploys/6646c024b95934000801803a

majetideepak

@rui-mo the implementation looks good to me. Left a couple of comments. Can you add a Parquet File with timestamp type to velox/dwio/parquet/tests/examples and add a test?

velox/dwio/parquet/reader/TimestampColumnReader.h

majetideepak · 2023-04-21T07:22:50Z

velox/dwio/parquet/reader/TimestampColumnReader.h

+namespace facebook::velox::parquet {
+
+class TimestampColumnReader : public IntegerColumnReader {
+ public:


Can we create a struct int96 {int val[3];}; in this class and use that instead of allocation int128_t?

Just char[12] (or a local using int96_t = char[12]) would be good I think

Hi @Yuhta @majetideepak, I tried above two ways, but I have to say a lot of compilation errors were encountered.

For char[12] way, below error was found during compilation.

../.././velox/dwio/common/SelectiveColumnReaderInternal.h:55:39: required from ‘void facebook::velox::dwio::common::SelectiveColumnReader::ensureValuesCapacity(facebook::velox::vector_size_t) [with T = char [12]; facebook::velox::vector_size_t = int]’ ../.././velox/dwio/common/SelectiveColumnReaderInternal.h:100:3: required from ‘void facebook::velox::dwio::common::SelectiveColumnReader::prepareRead(facebook::velox::vector_size_t, facebook::velox::dwio::common::RowSet, const uint64_t*) [with T = char [12]; facebook::velox::vector_size_t = int; facebook::velox::dwio::common::RowSet = folly::Range<const int*>; uint64_t = long unsigned int]’ ../.././velox/dwio/parquet/reader/TimestampColumnReader.h:41:47: required from here /usr/include/c++/9/optional:952:2: error: function returning an array 952 | value_or(_Up&& __u) const& | ^~~~~~~~ /usr/include/c++/9/optional:963:2: error: function returning an array 963 | value_or(_Up&& __u) && | ^~~~~~~~ In file included from ../../velox/dwio/parquet/reader/ParquetColumnReader.cpp:22: ../.././velox/dwio/common/SelectiveColumnReaderInternal.h: In instantiation of ‘void facebook::velox::dwio::common::SelectiveColumnReader::ensureValuesCapacity(facebook::velox::vector_size_t) [with T = char [12]; facebook::velox::vector_size_t = int]’: ../.././velox/dwio/common/SelectiveColumnReaderInternal.h:100:3: required from ‘void facebook::velox::dwio::common::SelectiveColumnReader::prepareRead(facebook::velox::vector_size_t, facebook::velox::dwio::common::RowSet, const uint64_t*) [with T = char [12]; facebook::velox::vector_size_t = int; facebook::velox::dwio::common::RowSet = folly::Range<const int*>; uint64_t = long unsigned int]’ ../.././velox/dwio/parquet/reader/TimestampColumnReader.h:41:47: required from here ../.././velox/dwio/common/SelectiveColumnReaderInternal.h:55:39: note: when instantiating default argument for call to ‘static facebook::velox::BufferPtr facebook::velox::AlignedBuffer::allocate(size_t, facebook::velox::memory::MemoryPool*, const std::optional<_Tp>&) [with T = char [12]; facebook::velox::BufferPtr = boost::intrusive_ptr<facebook::velox::Buffer>; size_t = long unsigned int]’ 55 | values_ = AlignedBuffer::allocate<T>( | ~~~~~~~~~~~~~~~~~~~~~~~~~~^ 56 | numRows + (simd::kPadding / sizeof(T)), &memoryPool_); | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

For struct int96 {int val[3];}; way, I solved some compilation errors and ended up with an error of missing conversion from int64_t to int96_t at RLEv1.h. Because I create a columnar visitor in SelectiveIntegerColumnReader.h with type int96_t (the self-defined struct), while value is of int64_t type.

case 12: reinterpret_cast<Reader*>(this)->Reader::readWithVisitor( rows, ColumnVisitor<int96_t, TFilter, ExtractValues, isDense>( *reinterpret_cast<TFilter*>(filter), this, rows, extractValues)); break;

Looks more work is needed here to support a self-defined int96 type, and some template functions for integer need to be changed.
Do you have any suggestion here?

Can we consider using int128_t as an alternative first? Because it helps simply the implementation.

I don't think using int128_t will work here, the valueSize_ is different and you will end up reading different part of data and even read out of bound.

Maybe check #2822 as a reference for what need to be done.

@Yuhta Thank you. I see. The main problem here is __int128 is a built-in type in GCC while int96 is not there, so to use int96_t requires more work. For example, the implicit conversions between int64_t and int96_t, and some operators like * and += etc. are lacking. Therefore, a lot of compiling errors occur in the code path of IntegerColumnReader.
Do we intend to implement a robust int96 type, or try other solution if there is?

The actual underlying structure is not really a generic int96_t but this:

struct __attribute__((__packed__)) Int96Timestamp { int32_t days; uint64_t nanos; };

I would recommend you to make this type to work with IntegerColumnReader, that would be the easiest way I guess.

I will continue to work on that. Convert this PR as draft for now, and will ping you for review when it's ready. Thanks.

Yuhta

Please add some tests in https://github.com/facebookincubator/velox/blob/main/velox/dwio/parquet/tests/reader/E2EFilterTest.cpp

You probably need to make the writer to generate INT96 in writeToMemory

Yuhta · 2023-04-21T15:06:24Z

velox/dwio/parquet/reader/TimestampColumnReader.h

+namespace facebook::velox::parquet {
+
+class TimestampColumnReader : public IntegerColumnReader {
+ public:


Just char[12] (or a local using int96_t = char[12]) would be good I think

velox/dwio/parquet/reader/TimestampColumnReader.h

majetideepak · 2023-04-21T18:29:14Z

You probably need to make the writer to generate INT96 in writeToMemory

@Yuhta I doubt if the Arrow Bridge supports int96 type. But worth checking. The alternative is to check in a file.
Arrow Bridge has a similar issue with Parquet Decimal types backed by int64.

Yuhta · 2023-04-21T19:17:32Z

@majetideepak Using a fixed file gives less coverage, but if the writer is not working then we have to do it this way for now. Either way we should make sure the result is correct with or without filters.

rui-mo · 2023-04-23T02:27:49Z

@majetideepak @Yuhta Thanks for your review! Your comments are well received, and I'm working on them.

rui-mo · 2023-04-28T07:04:09Z

Please add some tests in https://github.com/facebookincubator/velox/blob/main/velox/dwio/parquet/tests/reader/E2EFilterTest.cpp

You probably need to make the writer to generate INT96 in writeToMemory

@Yuhta I also tried that. Use enable_deprecated_int96_timestamps can make the arrow writer generate INT96. Since int128_t is used in Timestamp reader for now, the decoder calls readInt128() but little endian is not support currently (see IntDecoder.h). I will continue to work on this after the type issue is decided.

velox/type/Timestamp.h

Yuhta · 2023-04-28T15:03:01Z

velox/dwio/parquet/writer/Writer.cpp

@@ -32,7 +32,9 @@ void Writer::write(const RowVectorPtr& data) {
      recordBatch->schema(), recordBatch->columns(), data->size());
  if (!arrowWriter_) {
    stream_ = std::make_shared<DataBufferSink>(pool_);
-    auto arrowProperties = ::parquet::ArrowWriterProperties::Builder().build();
+    auto arrowProperties = ::parquet::ArrowWriterProperties::Builder()


Can you pass in the ArrowWriterProperties in the constructor and enable int96 in the corresponding unit tests only?

Yuhta · 2023-04-28T15:24:20Z

velox/dwio/parquet/tests/reader/ParquetTableScanTest.cpp

+       Timestamp(1197417600, 16076000000000)});
+
+  loadData(
+      getExampleFilePath("timestamp-int96.parquet"),


Just use the writer to write the file in memory since you find a way to do it; also add some tests in https://github.com/facebookincubator/velox/blob/main/velox/dwio/parquet/tests/reader/E2EFilterTest.cpp for timestamp type

velox/dwio/parquet/reader/TimestampColumnReader.h

Yuhta · 2023-04-28T15:54:58Z

velox/dwio/parquet/reader/TimestampColumnReader.h

+namespace facebook::velox::parquet {
+
+class TimestampColumnReader : public IntegerColumnReader {
+ public:


I don't think using int128_t will work here, the valueSize_ is different and you will end up reading different part of data and even read out of bound.

Maybe check #2822 as a reference for what need to be done.

velox/vector/arrow/Bridge.cpp

rui-mo · 2023-05-18T08:09:52Z

I don't think using int128_t will work here, the valueSize_ is different and you will end up reading different part of data and even read out of bound.

hi @Yuhta, I spent more time on Int96Timestamp type support but it is not easy to make it work through.
We also made more tests on the int128_t workaround, and found it could work for a pure scan. As posted in this PR, int96 in Parquet is converted to Velox Timestamp type (which is of 16-byte length) in PageReader (see link), and only numValues * sizeof(Int96Timestamp) bytes of data was read in PageReader.

Below is the stack of current timestamp scan.

facebook::velox::parquet::PageReader::prepareDictionary(facebook::velox::parquet::thrift::PageHeader const&) in ./velox_dwio_parquet_table_scan_test
 1# facebook::velox::parquet::PageReader::seekToPage(long) in ./velox_dwio_parquet_table_scan_test
 2# facebook::velox::parquet::PageReader::rowsForPage(facebook::velox::dwio::common::SelectiveColumnReader&, bool, bool, folly::Range<int const*>&, unsigned long const*&) in ./velox_dwio_parquet_table_scan_test
 3# void facebook::velox::parquet::PageReader::readWithVisitor<facebook::velox::dwio::common::ColumnVisitor<__int128, facebook::velox::common::AlwaysTrue, facebook::velox::dwio::common::ExtractToReader<facebook::velox::dwio::common::SelectiveIntegerColumnReader>, true> >(facebook::velox::dwio::common::ColumnVisitor<__int128, facebook::velox::common::AlwaysTrue, facebook::velox::dwio::common::ExtractToReader<facebook::velox::dwio::common::SelectiveIntegerColumnReader>, true>&) in ./velox_dwio_parquet_table_scan_test

Could you explain more about the possible risks? Thank you.

Yuhta

I don't think using int128_t will work here, the valueSize_ is different and you will end up reading different part of data and even read out of bound.

hi @Yuhta, I spent more time on Int96Timestamp type support but it is not easy to make it work through. We also made more tests on the int128_t workaround, and found it could work for a pure scan. As posted in this PR, int96 in Parquet is converted to Velox Timestamp type (which is of 16-byte length) in PageReader (see link), and only numValues * sizeof(Int96Timestamp) bytes of data was read in PageReader.

Below is the stack of current timestamp scan.
facebook::velox::parquet::PageReader::prepareDictionary(facebook::velox::parquet::thrift::PageHeader const&) in ./velox_dwio_parquet_table_scan_test
 1# facebook::velox::parquet::PageReader::seekToPage(long) in ./velox_dwio_parquet_table_scan_test
 2# facebook::velox::parquet::PageReader::rowsForPage(facebook::velox::dwio::common::SelectiveColumnReader&, bool, bool, folly::Range<int const*>&, unsigned long const*&) in ./velox_dwio_parquet_table_scan_test
 3# void facebook::velox::parquet::PageReader::readWithVisitor<facebook::velox::dwio::common::ColumnVisitor<__int128, facebook::velox::common::AlwaysTrue, facebook::velox::dwio::common::ExtractToReader<facebook::velox::dwio::common::SelectiveIntegerColumnReader>, true> >(facebook::velox::dwio::common::ColumnVisitor<__int128, facebook::velox::common::AlwaysTrue, facebook::velox::dwio::common::ExtractToReader<facebook::velox::dwio::common::SelectiveIntegerColumnReader>, true>&) in ./velox_dwio_parquet_table_scan_test
Could you explain more about the possible risks? Thank you.

So the assumption here is it is always dictionary-encoded? If this assumption holds all the time, we can probably go this way. It's only a problem when we want to apply a filter on the column of flat values.

Make sure to beef up your E2E filter tests with filters on some primary keys (int64 is fine), and also put timestamp in complex types (array, map, struct) in addition to top-level column.

rui-mo · 2023-05-23T09:26:32Z

@Yuhta Thanks for your reply.

So the assumption here is it is always dictionary-encoded? If this assumption holds all the time, we can probably go this way. It's only a problem when we want to apply a filter on the column of flat values.

Understood the gap here. I guess RLEV1 and Plain encoding are also possible because the column encoding can be set during Parquet write, but we only tested the Parquet generated with default configs.

Make sure to beef up your E2E filter tests with filters on some primary keys (int64 is fine), and also put timestamp in complex types (array, map, struct) in addition to top-level column.

Got it, will do.

velox/vector/arrow/Bridge.cpp

velox/dwio/parquet/reader/PageReader.cpp

Summary: Arrow bridge supports different timestamp units, including second, milli, micro and nano. This PR adds `TimestampUnit` in `ArrowOptions` to support these units in the process of exportToArrow. For importFromArrow, the unit extracted from arrow schema is followed. By default, the conversion unit is nano, and in Gluten, micro is configured to align with Spark. #4680 (comment) Arrow Reference: https://github.com/apache/arrow/blob/main/cpp/src/arrow/c/bridge.cc#L402-L421. Pull Request resolved: #7625 Reviewed By: mbasmanova Differential Revision: D54852534 Pulled By: Yuhta fbshipit-source-id: 0494102fedc73f7068424bc09e972e7deb297a6e

yingsu00 · 2024-04-24T21:51:10Z

Related Presto issue prestodb/presto#22605

velox/type/Type.h

Yuhta · 2024-04-25T20:13:14Z

velox/connectors/hive/HiveDataSource.cpp

-      false,
-      filters,
-      sampleRate);
+  auto remainingFilter = hiveTableHandle_->remainingFilter();


Curious why do we need this? isFilterPushdownEnabled should be ignored in Prestissimo

It is just a workaround to get test work. Without this change, filter pushdown happens and below exception is observed.

C++ exception with description "Exception: VeloxUserError
Error Source: USER
Error Code: UNSUPPORTED
Reason: TimestampRange: [-292275055-05-16T16:47:04.000000000, 2000-09-12T22:36:28.999999999] no nulls: testInt128() is not supported.

This flag will be removed in the future. Will it work if we just implement TimestampRange::testInt128?

Makes sense. I will take a look. Thanks for the suggestion.

PHILO-HE · 2024-05-08T08:22:52Z

velox/dwio/parquet/reader/TimestampColumnReader.h

+    auto& data = formatData_->as<ParquetData>();
+    // Use int128_t as a workaroud. Timestamp in Velox is of 16-byte length.
+    prepareRead<int128_t>(offset, rows, nullptr);
+    readCommon<IntegerColumnReader>(rows);


@rui-mo, I note an extra template arg kEncodingHasNulls is added by 0c4dad1.

I just use true for it in a downstream branch.
oap-project@a4039c2#diff-ae87451c1577f3b47d2863187de8bf30c7351484d39537419016487cc7b2f71cR44

Updated, thanks.

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Apr 20, 2023

rui-mo mentioned this pull request Apr 20, 2023

Timestamp reader support for Parquet file format #4681

Open

Yuhta self-requested a review April 20, 2023 15:13

rui-mo force-pushed the wip_ts_reader branch from f402fee to ad309ea Compare April 21, 2023 02:17

majetideepak reviewed Apr 21, 2023

View reviewed changes

Yuhta reviewed Apr 21, 2023

View reviewed changes

rui-mo force-pushed the wip_ts_reader branch 3 times, most recently from f5c944e to a8dee34 Compare April 28, 2023 06:15

rui-mo commented Apr 28, 2023

View reviewed changes

velox/type/Timestamp.h Outdated Show resolved Hide resolved

rui-mo force-pushed the wip_ts_reader branch from a8dee34 to 0d3ef7e Compare April 28, 2023 07:31

Yuhta reviewed Apr 28, 2023

View reviewed changes

rui-mo marked this pull request as draft May 8, 2023 05:35

zeodtr reviewed May 16, 2023

View reviewed changes

velox/vector/arrow/Bridge.cpp Outdated Show resolved Hide resolved

rui-mo force-pushed the wip_ts_reader branch 6 times, most recently from 53d9408 to 1fe1694 Compare May 18, 2023 07:03

Yuhta self-requested a review May 18, 2023 15:25

Yuhta reviewed May 18, 2023

View reviewed changes

rui-mo force-pushed the wip_ts_reader branch from 1fe1694 to cd38df2 Compare August 30, 2023 03:59

rui-mo force-pushed the wip_ts_reader branch 3 times, most recently from 828d05d to 3990c66 Compare November 22, 2023 06:18

rui-mo mentioned this pull request Dec 12, 2023

Add support to read plain encoded INT96 timestamp from Parquet file oap-project/velox#456

Closed

rui-mo force-pushed the wip_ts_reader branch from 3990c66 to 107c963 Compare January 2, 2024 09:47

mskapilks mentioned this pull request Jan 10, 2024

Support for plain and dictionary encoded INT64 timestamp in parquet files #8325

Open

liujiayi771 reviewed Feb 19, 2024

View reviewed changes

velox/vector/arrow/Bridge.cpp Outdated Show resolved Hide resolved

velox/dwio/parquet/reader/PageReader.cpp Outdated Show resolved Hide resolved

rui-mo force-pushed the wip_ts_reader branch from 107c963 to 15d35cc Compare February 20, 2024 02:46

rui-mo force-pushed the wip_ts_reader branch from 15d35cc to 9b340df Compare March 5, 2024 05:20

rui-mo mentioned this pull request Mar 26, 2024

Nanos in timestamp INT96 can be negative oap-project/velox#485

Open

rui-mo force-pushed the wip_ts_reader branch 3 times, most recently from dde7df4 to 1c78db0 Compare March 26, 2024 12:09

rui-mo mentioned this pull request Mar 26, 2024

Nanos in timestamp INT96 can be negative oap-project/velox#486

Merged

rui-mo force-pushed the wip_ts_reader branch from 1c78db0 to 1df8bf1 Compare March 27, 2024 02:18

8dukongjian mentioned this pull request Apr 22, 2024

Summary of Parquet reader Issues #9560

Open

yingsu00 mentioned this pull request Apr 24, 2024

Presto Parquet writer writes timestamp in 96 bits width prestodb/presto#22605

Open

Yuhta reviewed Apr 25, 2024

View reviewed changes

velox/type/Type.h Outdated Show resolved Hide resolved

Yuhta reviewed Apr 25, 2024

View reviewed changes

rui-mo changed the title ~~Support timestamp reader for Parquet file format~~ Support INT96 timestamp read for dictionary-encoded Parquet Apr 26, 2024

rui-mo changed the title ~~Support INT96 timestamp read for dictionary-encoded Parquet~~ Support for dictionary encoded INT96 timestamp in parquet files Apr 26, 2024

rui-mo force-pushed the wip_ts_reader branch from 1df8bf1 to 63ce5f6 Compare April 26, 2024 09:27

PHILO-HE reviewed May 8, 2024

View reviewed changes

rui-mo and others added 3 commits May 17, 2024 08:59

Support timestamp reader

432d1b2

Fix negative nano

518bbdd

update

8a3df69

rui-mo force-pushed the wip_ts_reader branch 2 times, most recently from cef49e2 to 8a3df69 Compare May 17, 2024 02:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for dictionary encoded INT96 timestamp in parquet files #4680

Support for dictionary encoded INT96 timestamp in parquet files #4680

rui-mo commented Apr 20, 2023 •

edited

netlify bot commented Apr 20, 2023 •

edited

majetideepak left a comment

majetideepak Apr 21, 2023 •

edited

Yuhta Apr 21, 2023

rui-mo Apr 28, 2023

rui-mo Apr 28, 2023

Yuhta Apr 28, 2023

rui-mo May 5, 2023 •

edited

Yuhta May 5, 2023

rui-mo May 8, 2023

Yuhta left a comment

Yuhta Apr 21, 2023

majetideepak commented Apr 21, 2023 •

edited

Yuhta commented Apr 21, 2023

rui-mo commented Apr 23, 2023

rui-mo commented Apr 28, 2023

Yuhta Apr 28, 2023

Yuhta Apr 28, 2023

Yuhta Apr 28, 2023

rui-mo commented May 18, 2023 •

edited

Yuhta left a comment •

edited

rui-mo commented May 23, 2023

yingsu00 commented Apr 24, 2024

Yuhta Apr 25, 2024

rui-mo Apr 26, 2024

Yuhta Apr 26, 2024

rui-mo Apr 28, 2024

PHILO-HE May 8, 2024

rui-mo May 17, 2024

Support for dictionary encoded INT96 timestamp in parquet files #4680

Are you sure you want to change the base?

Support for dictionary encoded INT96 timestamp in parquet files #4680

Conversation

rui-mo commented Apr 20, 2023 • edited

netlify bot commented Apr 20, 2023 • edited

✅ Deploy Preview for meta-velox canceled.

majetideepak left a comment

Choose a reason for hiding this comment

majetideepak Apr 21, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rui-mo May 5, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Yuhta left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

majetideepak commented Apr 21, 2023 • edited

Yuhta commented Apr 21, 2023

rui-mo commented Apr 23, 2023

rui-mo commented Apr 28, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rui-mo commented May 18, 2023 • edited

Yuhta left a comment • edited

Choose a reason for hiding this comment

rui-mo commented May 23, 2023

yingsu00 commented Apr 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rui-mo commented Apr 20, 2023 •

edited

netlify bot commented Apr 20, 2023 •

edited

majetideepak Apr 21, 2023 •

edited

rui-mo May 5, 2023 •

edited

majetideepak commented Apr 21, 2023 •

edited

rui-mo commented May 18, 2023 •

edited

Yuhta left a comment •

edited