Making TimestampWithTimezone a primitive type

[kagamiori]

TimestampWithTimeZone is a primitive type in Presto. In contrast, it is implemented as a Row<int64_t, int16_t> in Velox and hence treated as a complex type implicitly. This discussion aims at exploring the possibility of making TimestampWithTimezone in Velox a primitive type and the tradeoff of this potential change.

Motivation
Making TimestampWithTimezone a primitive type brings better consistency between Velox and Presto, and reduces the chance of errors. For example, a TimestampWithTimezone value can be either NULL or non-NULL. But with the current implementation as Row<int64_t, int16_t>, it is possible to create a TimestampWithTimezone vector where only the first or the second field of a value is NULL. This is unexpected and likely not handled by existing UDFs.

Changes needed
To make TimestampWithTimezone primitive, we need to redefine the type for it. After that, we need to update datetime functions that have TimestampWithTimezone argument or return types. We also need to add support for serializing and deserializing TimestampWithTimezone vectors.

[kagamiori]

CC @mbasmanova

[pedroerp]

Do you mean by "making TimestampWithTimezone primitive" that it would stop using the type extensibility framework?

In general, we try only adding to the main library the types (but also functions, aggregates, operators) that are sufficiently generic to be used by more than one system integrating with Velox. Is the TimestampWithTimezone type Presto-specific?

[mbasmanova]

It might be worth revisiting the decision to store timestamps with nanosecond precision. These are not supported by Presto or Spark and Substrait also supports only microsecond precision.

https://substrait.io/types/type_classes/

timestamp_tz | A timezone-aware timestamp within [1000-01-01 00:00:00.000000 UTC..9999-12-31 23:59:59.999999 UTC], with microsecond precision. Similar to aware datetime in Python.

[majetideepak]

@kagamiori, @mbasmanova, we should prioritize this work since that will simplify Parquet Reader timestamp and timestamptz support. #4680
Are there a lot of Meta internal changes if we go ahead with this work? I want to make sure we have the resources to time this.

[mbasmanova]

CC: @wypb @aditi-pandit

[wypb]

Got it

[wypb]

Hi @mbasmanova @majetideepak @aditi-pandit Are you working on this? If not, I can do this, because I have also been working on TimeWithTimeZone type recently, and it should be similar to this.

[mbasmanova]

@wypb I don't believe anyone is working on this. You are welcome to pick it up. Start by describing the changes you planning to make.

[aditi-pandit]

@wypb I don't believe anyone is working on this. You are welcome to pick it up. Start by describing the changes you planning to make.

+1. Please describe the changes you are planning to make and we can take it further.

[mbasmanova]

@wypb Thank you for putting together this design doc. Overall looks good.

@spershin @amitkdutta Amit, Sergey, FYI, this will most likely be a breaking change that cannot be made backwards-compatible.

[aditi-pandit]

@wypb : Thanks for the doc. Did a first pass. Looks good overall.

[majetideepak]

@mbasmanova, @wypb Supporting different precisions for timestamp(nanos) and timestampTz(micros) will lead to odd semantics. For example, we will see different sort results after converting a timestamp column to timestampTz column since the precision will be truncated. I have not seen a system where these two types have different precision.
Can we consider using the same physical type for timestamp and timestampTz?

[ViggoC]

@majetideepak In Arrow, both timestamp and timestampTz support three levels of precision: milliseconds, microseconds, and nanoseconds. Is Velox considering supporting all three as well?

[majetideepak]

Velox Timestamp supports Millis and Nanos. We could support Micros without much effort.
https://github.com/facebookincubator/velox/blob/main/velox/type/Timestamp.h
Arrow's timestampTz is different from Presto's TimestampWithTimezone. In Arrow, the timestampTz is a logical type with timezone information set as part of the type. https://github.com/apache/arrow/blob/main/cpp/src/arrow/type.h#L1664
Presto's TimestampWithTimezone follows the SQL semantics where the timezone is part of the value. Supporting microsecond and nanoseconds is not feasible since we are making it a native type backed by BigInt.

[majetideepak]

@mbasmanova Don't we want to fix the TIMESTAMP scalar type in Velox as well in this fashion? That was my understanding.
Timestamp and TimestampTz can both be logical types backed by BIGINT.

[majetideepak]

I found this blog helpful. https://www.databricks.com/blog/2020/07/22/a-comprehensive-look-at-dates-and-timestamps-in-apache-spark-3-0.html
There are three types of timestamp types.
Timestamp Without Time Zone, Timestamp With Time Zone, Timestamp With Session Time Zone.
Spark/Parquet support the third type. I think we can support both Timestamp Without Time Zone and Timestamp With Session Time Zone using the Timestamp Type in Velox.
We need a Timestamp with Time Zone type to match Presto spec.

[mbasmanova]

@majetideepak Deepak, this is a nice blog post. Thank you for sharing. I assume there is no confusion left at this point. Let me know if that's not the case.

[majetideepak]

@mbasmanova, I am trying to map the above three timestamp types to Presto types. I think Presto's Timestamp With Timezone semantics is broken for Java Parquet. Technically, Parquet/ORC cannot support Timestamp with Timezone since they do not store the timezone information. Parquet only supports Timestamp Without Timezone and Timestamp With Session Timezone.
However, Presto supports Timestamp With Session Timezone and Timestamp With Timezone. Is this accurate with DWRF as well?
Velox's timestamp type semantics are also not clear. Is it Timestamp Without Timezone or Timestamp With Session Timezone? It could be just a physical type and leave the semantics to the client application. We should clarify this.

[majetideepak]

Looking at the Presto Java code further, it looks like we simply add the UTC timezone to Parquet values normalized to UTC. The semantics are not broken.

[mbasmanova]

Yes, I'd expect that we convert timestamps to UTC when storing in Parquet / DWRF files.

[majetideepak]

I also just noticed that the Presto Timestamp With Timezone says

Instant in time that includes the date and time of day with a time zone. 
Values of this type are rendered using the time zone from the value.

Example: TIMESTAMP '2001-08-22 03:04:05.321 America/Los_Angeles'

But none of the Hive file formats support a TimezoneTz type where the timezone is part of the value.
So we should be fine to break the semantics here.

[majetideepak]

There is no semantic being broken here. I misunderstood Velox's TimestampTz specification.

[mbasmanova]

FYI, there is a correctness issue for queries that use this type: #10338