VectorHasher can lose timestamp precision in generic value-id paths

[rui-mo]

Description

VectorHasher currently supports value IDs for TIMESTAMP, but its timestamp normalization is millisecond-based.

velox/velox/exec/VectorHasher.h

Lines 410 to 412 in 2ca5030

	inline int64_t toInt64(Timestamp timestamp) const {
	return timestamp.toMillis();
	}

The direct valueId(Timestamp) and lookupValueId(Timestamp) overloads correctly reject timestamps whose nanoseconds are not millisecond-aligned. But some generic paths bypass these overloads and call toInt64(Timestamp) directly.

velox/velox/exec/VectorHasher.h

Lines 774 to 785 in 2ca5030

	template <>
	inline uint64_t VectorHasher::valueId(Timestamp value) {
	if (FOLLY_UNLIKELY(
	value.getNanos() % Timestamp::kNanosecondsInMillisecond != 0)) {
	// The timestamp is in nanosecond or microsecond precision. The values are
	// not mappable to milliseconds without precision loss.
	setRangeOverflow();
	setDistinctOverflow();
	return kUnmappable;
	}
	return valueId(value.toMillis());
	}

velox/velox/exec/VectorHasher.h

Lines 763 to 768 in 2ca5030

	template <>
	inline uint64_t VectorHasher::lookupValueId(Timestamp timestamp) const {
	return timestamp.getNanos() % 1'000'000 != 0
	? kUnmappable
	: lookupValueId(timestamp.toMillis());
	}

In particular:
VectorHasher::tryMapToRange<T>() uses toInt64(values[row]) in the range fast path.
VectorHasher::analyzeValue<T>() uses toInt64(value) when collecting row-wise stats.
Overflow state from an unmappable timestamp can also be lost during merge() if the overflowed hasher is treated as empty after its range/distinct stats are cleared.

This can cause distinct timestamp values to be assigned the same value id. For example:

Timestamp::fromMillis(1)
Timestamp::fromMicros(1'001)

Current Limitation

Spark timestamps have microsecond precision so timestamp join or aggregation keys may commonly be not millisecond-aligned. This means Spark timestamp keys with microsecond precision cannot benefit from value-id / normalized-key / array hash-table fast paths.

Discussion

1. Would it make sense to make timestamp value-id precision configurable?

Possible modes:

• milliseconds: current default behavior;
• microseconds: useful for Spark-compatible execution.

2. Could this be a query config and a VectorHasher constructor option?

[rui-mo]

Hi @pedroerp, following up on your comment, I added my findings on the timestamp issue in VectorHasher.

I filed #18598 to address the potential correctness issue with the current millisecond-based precision. I also added some discussion to the issue about whether the timestamp value-ID precision should be configurable, so that the fast paths can also be enabled for microsecond precision, which is used by Spark.

Happy to hear your opinions, thanks!