[Parquet] TimestampPrecision / TimestampUnit mismatch in read / write files, particular for unit tests

[zuyu]

Bug description

https://github.com/facebookincubator/velox/blob/473902a63ed6a8bac85a2f598b360de739f3bfe4/velox/type/Timestamp.h#L33-L37

https://github.com/facebookincubator/velox/blob/473902a63ed6a8bac85a2f598b360de739f3bfe4/velox/vector/arrow/Bridge.h#L28-L33

• TimestampUnit::kSecond does not have a match in TimestampPrecision
• default values mismatch: TimestampPrecision::kMilliseconds used in read vs TimestampUnit::kNano used in write; most importantly, unit tests (i.e., timestamp int96) does not set the values.
  • read path: https://github.com/facebookincubator/velox/blob/473902a63ed6a8bac85a2f598b360de739f3bfe4/velox/dwio/common/Options.h#L426 https://github.com/facebookincubator/velox/blob/473902a63ed6a8bac85a2f598b360de739f3bfe4/velox/connectors/hive/HiveConnectorUtil.cpp#L624-L625 https://github.com/facebookincubator/velox/blob/473902a63ed6a8bac85a2f598b360de739f3bfe4/velox/connectors/hive/HiveConfig.cpp#L264-L267
  • write path: https://github.com/facebookincubator/velox/blob/473902a63ed6a8bac85a2f598b360de739f3bfe4/velox/vector/arrow/Bridge.h#L38 https://github.com/facebookincubator/velox/blob/473902a63ed6a8bac85a2f598b360de739f3bfe4/velox/dwio/parquet/writer/Writer.h#L107 https://github.com/facebookincubator/velox/blob/473902a63ed6a8bac85a2f598b360de739f3bfe4/velox/dwio/parquet/writer/Writer.cpp#L241-L242

Proposed Fixes

• Introduce a new kNotSet as the default value, and requires setting both TimestampPrecision and TimestampUnit if reading / writing a timestamp column. Otherwise, an assertion VELOX_UNREACHABLE() would trigger.
• For timestamp-related unit tests, need to align the values for both TimestampPrecision and TimestampUnit.

[Yuhta]

I would say just align them. Adding kNotSet will make the thing unnecessarily complicated.

[zuyu]

@Yuhta How about TimestampUnit::kSecond, remove it or add TimestampPrecision::kSecond? I prefer to removing it, as it equals to that nanos is 0.

[Yuhta]

@zuyu We can remove it if neither Presto or Spark support it