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

[zuyu]

Bug description

velox/velox/type/Timestamp.h

Lines 33 to 37 in 473902a

	enum class TimestampPrecision : int8_t {
	kMilliseconds = 3, // 10^3 milliseconds are equal to one second.
	kMicroseconds = 6, // 10^6 microseconds are equal to one second.
	kNanoseconds = 9, // 10^9 nanoseconds are equal to one second.
	};

velox/velox/vector/arrow/Bridge.h

Lines 28 to 33 in 473902a

	enum class TimestampUnit : uint8_t {
	kSecond = 0 /*10^0 second is equal to 1 second*/,
	kMilli = 3 /*10^3 milliseconds are equal to 1 second*/,
	kMicro = 6 /*10^6 microseconds are equal to 1 second*/,
	kNano = 9 /*10^9 nanoseconds are equal to 1 second*/
	};

• 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:

    velox/velox/dwio/common/Options.h

    Line 426 in 473902a

    TimestampPrecision timestampPrecision_ = TimestampPrecision::kMilliseconds;

    velox/velox/connectors/hive/HiveConnectorUtil.cpp

    Lines 624 to 625 in 473902a

    	rowReaderOptions.setTimestampPrecision(static_cast<TimestampPrecision>(
    	hiveConfig->readTimestampUnit(sessionProperties)));

    velox/velox/connectors/hive/HiveConfig.cpp

    Lines 264 to 267 in 473902a

    	uint8_t HiveConfig::readTimestampUnit(const config::ConfigBase* session) const {
    	const auto unit = session->get<uint8_t>(
    	kReadTimestampUnitSession,
    	config_->get<uint8_t>(kReadTimestampUnit, 3 /*milli*/));

  • write path:

    velox/velox/vector/arrow/Bridge.h

    Line 38 in 473902a

    TimestampUnit timestampUnit = TimestampUnit::kNano;

    velox/velox/dwio/parquet/writer/Writer.h

    Line 107 in 473902a

    std::optional<TimestampUnit> parquetWriteTimestampUnit;

    velox/velox/dwio/parquet/writer/Writer.cpp

    Lines 241 to 242 in 473902a

    	options_.timestampUnit =
    	options.parquetWriteTimestampUnit.value_or(TimestampUnit::kNano);

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