Timestamp type in Velox

[liujiayi771]

I find that TimestampType in Presto stores timestamp in long value with a TimeUnit. Spark also stores timestamp in long value with MICROSECONDS unit. But Velox uses seconds and nanos to represent timestamp, could you tell me the reason?

[liujiayi771]

Hi, @Yuhta @mbasmanova @majetideepak There are some other questions about the current Timestamp.

• According to Timestamp's comparison function, is Timestamp(10, 0) > Timestamp(9, 2*10^9)? Or we need to round the nanos to seconds?
• According to some unit test, for example, EXPECT_EQ(59, second(Timestamp(-1, 12300000000)));, Can we just ignore the nanos part (result is 59), or we need to round the nanos to seconds(result is 11)?

[rui-mo]

If rounding nanos to seconds is needed during comparison, is it recommended to do that in the Timestamp constructor, or just in the comparison functions?

[Yuhta]

I would say either round it or check it is in range in the constructor

[liujiayi771]

I find that Velox define the maximum value of nanos in IntCodecCommon.h, but never check this in Timestamp type. Should we add this restriction to the Timestamp type? In this way, there is no need to consider the problem of round nanos to seconds.

velox/velox/dwio/common/IntCodecCommon.h

Line 42 in f72d6c6

constexpr int64_t MAX_NANOS = 999'999'999;

[mbasmanova]

@liujiayi771 @rui-mo The precision of timestamp values in Presto, Spark and Velox is different. One option is to define Presto-specific and Spark-specific timestamp types as logical types backed by BIGINT physical type. Adding a check for nanos to Timestamp constructor also might be helpful. Finally, it would be nice to update documentation at https://facebookincubator.github.io/velox/develop/types.html to explain how to use Velox's Timestamp type in Presto and Spark.

[liujiayi771]

Thanks, @mbasmanova . First, I will try to add a check in the constructor of Timestamp to ensure the value range of nanos. There are some codes in Velox that do not consider the value range of nanos, I will try to fix it.

[liujiayi771]

Hi @mbasmanova Do you think we should just limit the value of nanos, or allow the input of larger nanos, and correct it by rounding nanos to seconds?

[mbasmanova]

Do you think we should just limit the value of nanos, or allow the input of larger nanos, and correct it by rounding nanos to seconds?

I don't have a strong opinion, but generally prefer check + fail rather then silent adjust behaviors.