ARROW-4908: [Rust] [DataFusion] Add support for date/time parquet types encoded as INT32/INT64 #3940

andygrove · 2019-03-16T21:16:49Z

This PR adds support for the date/time parquet types that can be encoded in INT32/INT64 physical types.

andygrove · 2019-03-16T21:18:06Z

@nevi-me @liurenjie1024 Could you review please

codecov-io · 2019-03-16T21:59:20Z

Codecov Report

Merging #3940 into master will increase coverage by 1.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #3940      +/-   ##
==========================================
+ Coverage   87.75%   88.77%   +1.01%     
==========================================
  Files         727      594     -133     
  Lines       89499    81603    -7896     
  Branches     1252        0    -1252     
==========================================
- Hits        78541    72443    -6098     
+ Misses      10840     9160    -1680     
+ Partials      118        0     -118

Impacted Files	Coverage Δ
cpp/src/arrow/testing/random.cc	`97.59% <0%> (-2.41%)`	⬇️
cpp/src/arrow/util/bpacking.h	`99.81% <0%> (-0.01%)`	⬇️
cpp/src/parquet/encoding-test.cc	`100% <0%> (ø)`	⬆️
cpp/src/arrow/array-test.cc	`100% <0%> (ø)`	⬆️
cpp/src/arrow/testing/random.h	`100% <0%> (ø)`	⬆️
cpp/src/arrow/ipc/test-common.h	`98.25% <0%> (ø)`	⬆️
go/arrow/math/uint64_amd64.go
go/arrow/memory/memory_avx2_amd64.go
js/src/enum.ts
go/arrow/array/builder.go
... and 139 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 04a7b68...4e99e42. Read the comment docs.

nevi-me

Thanks @andygrove, I've requested changes to align with Arrow and Parquet's spec(s)

nevi-me · 2019-03-16T22:12:32Z

rust/arrow/src/datatypes.rs

@@ -195,6 +195,13 @@ make_type!(
    0i64
 );
 make_type!(Date32Type, i32, DataType::Date32(DateUnit::Day), 32, 0i32);
+make_type!(


The spec only provides for Date32 to have a day dateunit (http://arrow.apache.org/docs/format/Metadata.html#date). Parquet dates I believe are only represented in 32-bit number of days (https://github.com/apache/parquet-format/blob/master/LogicalTypes.md#date)

According to arrow spec, date with milliseconds should be 64 bit?

According to parquet, there is no date type with milliseconds units?

So I think this may be unnecessary.

nevi-me · 2019-03-16T22:14:55Z

rust/datafusion/src/datasource/parquet.rs

-                        ColumnReader::Int32ColumnReader(ref mut r) => {
-                            ArrowReader::<Int32Type>::read(
+                        ColumnReader::Int32ColumnReader(ref mut r) => match dt {
+                            DataType::Date32(DateUnit::Millisecond) => {


If this condition's ever true, then it might be that we've incorrectly implemented the spec in the parquet crate. @sunchao am I misinterpreting the logical types spec?

@nevi-me what do you mean? int32 when used as date, represents the number of days since epoch. See https://github.com/apache/parquet-format/blob/master/LogicalTypes.md#datetime-types.

Sorry, I meant that the match arm for date 32 with milli should never evaluate to true

Yes, agreed. This should never happen.

nevi-me · 2019-03-16T22:17:20Z

rust/datafusion/src/datasource/parquet.rs

+                            )?,
+                        },
+                        ColumnReader::Int64ColumnReader(ref mut r) => match dt {
+                            DataType::Time64(TimeUnit::Microsecond) => {


There's aso Time64(Nanoseconds)

Agree with @nevi-me , Time64(Nanoseconds) is missing.

nevi-me · 2019-03-16T22:20:29Z

rust/datafusion/src/datasource/parquet.rs

+                                    is_nullable,
+                                )?
+                            }
+                            DataType::Timestamp(TimeUnit::Millisecond) => {


Please add TimestampNano. This will cover the successor of the deprecated int96 type

Do you know where I can get a parquet file with all the valid types so I can write comprehensive unit tests for this?

maybe try https://github.com/apache/parquet-testing/blob/master/data/alltypes_plain.parquet

this is the schema for "alltypes"

"id: Int32\n\ bool_col: Boolean\n\ tinyint_col: Int32\n\ smallint_col: Int32\n\ int_col: Int32\n\ bigint_col: Int64\n\ float_col: Float32\n\ double_col: Float64\n\ date_string_col: Utf8\n\ string_col: Utf8\n\ timestamp_col: Timestamp(Nanosecond)"

The only date/time is the INT96 one

This seems like a good question for the mailing list

andygrove · 2019-03-17T16:07:34Z

I've updated the parquet data source to handle all date/time types, but the nanosecond readers can never be hit because the parquet crate does not support nanosecond resolution for time or timestamp (see https://github.com/apache/arrow/blob/master/rust/parquet/src/basic.rs#L91-L105 and https://github.com/apache/arrow/blob/master/rust/parquet/src/reader/schema.rs#L215-L221).

nevi-me · 2019-03-17T16:58:04Z

Thanks @andygrove , that makes sense. We don't yet support v2.6.0 of the spec, which nanosecond came with (apache/parquet-format@b879065#diff-a557947b7d64dfe5f41d51e1acff9f52).

The parquet-format crate is at v2.6.0 (sunchao/parquet-format-rs#3) but parquet is still at v2.5.0. I think there might be some work other than timestamps that would need to be done to support the format, so it might not be trivial to bump up support.

I hadn't thought of that when I expected nano precision, my apologies for that.

andygrove · 2019-03-17T18:34:08Z

@nevi-me Ah, OK, thanks. I didn't know that either. Well, at least this part is ready for whenever the parquet crate gets updated.

I would have liked to add a parquet file with all types for comprehensive unit testing but I don't think I'm going to be able to do that in time for the 0.13.0 release since I'm traveling next week. I will create a JIRA for that.

liurenjie1024 · 2019-03-17T23:53:41Z

LGTM

andygrove · 2019-03-18T04:07:25Z

CI failure is not related to Rust

Add support for date/time parquet types encoded as INT32/INT64

54cce4b

andygrove requested a review from sunchao March 16, 2019 21:16

andygrove added the Component: Rust label Mar 16, 2019

nevi-me requested changes Mar 16, 2019

View reviewed changes

Update parquet data source for all date/time types

503151a

nevi-me approved these changes Mar 17, 2019

View reviewed changes

formatting

4e99e42

kszucs closed this in 129b77c Mar 18, 2019

asfimport mentioned this pull request Mar 18, 2019

[Rust] [DataFusion] Add support for parquet date/time in int32/64 encoding #21417

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ARROW-4908: [Rust] [DataFusion] Add support for date/time parquet types encoded as INT32/INT64 #3940

ARROW-4908: [Rust] [DataFusion] Add support for date/time parquet types encoded as INT32/INT64 #3940

andygrove commented Mar 16, 2019 •

edited

andygrove commented Mar 16, 2019

codecov-io commented Mar 16, 2019 •

edited

nevi-me left a comment

nevi-me Mar 16, 2019

liurenjie1024 Mar 17, 2019

nevi-me Mar 16, 2019

sunchao Mar 16, 2019

nevi-me Mar 17, 2019

sunchao Mar 17, 2019

nevi-me Mar 16, 2019

liurenjie1024 Mar 17, 2019

nevi-me Mar 16, 2019

andygrove Mar 16, 2019

sunchao Mar 16, 2019

andygrove Mar 16, 2019

andygrove Mar 16, 2019

andygrove commented Mar 17, 2019

nevi-me commented Mar 17, 2019 •

edited

andygrove commented Mar 17, 2019

liurenjie1024 commented Mar 17, 2019

andygrove commented Mar 18, 2019

ARROW-4908: [Rust] [DataFusion] Add support for date/time parquet types encoded as INT32/INT64 #3940

ARROW-4908: [Rust] [DataFusion] Add support for date/time parquet types encoded as INT32/INT64 #3940

Conversation

andygrove commented Mar 16, 2019 • edited

andygrove commented Mar 16, 2019

codecov-io commented Mar 16, 2019 • edited

Codecov Report

nevi-me left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andygrove commented Mar 17, 2019

nevi-me commented Mar 17, 2019 • edited

andygrove commented Mar 17, 2019

liurenjie1024 commented Mar 17, 2019

andygrove commented Mar 18, 2019

andygrove commented Mar 16, 2019 •

edited

codecov-io commented Mar 16, 2019 •

edited

nevi-me commented Mar 17, 2019 •

edited