Convert TIMESTAMP_WITH_TIME_ZONE to primitive type #7480

wypb · 2023-11-09T03:44:54Z

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 PR convert TIMESTAMP_WITH_TIME_ZONE
to primitive type to achive better consistency between Velox and Presto,
details of the discussion can be found here.

CC: @mbasmanova @aditi-pandit @majetideepak @kagamiori

netlify · 2023-11-09T03:45:00Z

✅ Deploy Preview for meta-velox canceled.

Name	Link
🔨 Latest commit	`5caa663`
🔍 Latest deploy log	https://app.netlify.com/sites/meta-velox/deploys/65d7ec3ff085540008274b20

velox/functions/prestosql/types/TimestampWithTimeZoneType.h

majetideepak · 2023-11-09T16:01:00Z

This change will also break the Presto TimestampTz semantics as mentioned here #2511 (comment)
But this change helps optimize TimestampTz type from Hive file formats. Let's make that clear in the description.

wypb · 2023-11-10T02:22:06Z

This change will also break the Presto TimestampTz semantics as mentioned here #2511 (comment)

I don't quite understand this. The implementation of TimestampTz in this PR is represented by bigint, which contains timestamp and time zone, I think it is consistent with this this implementation in Presto.

majetideepak · 2023-11-10T05:02:37Z

@wypb you are right. I misunderstood Java's TimestampTz specification.
After reading some of the Java code, the semantics do match. Thanks!

wypb · 2023-11-16T10:33:14Z

Hi @kagamiori @aditi-pandit @mbasmanova Could you also help me review this, thank you.

wypb · 2023-11-22T23:52:59Z

Hi @kagamiori Could you also help me review this, thank you.

wypb · 2023-11-28T11:47:43Z

Hi @kagamiori @aditi-pandit @mbasmanova Any comments on this PR?

wypb · 2023-12-12T10:26:31Z

Hi @mbasmanova @kagamiori @aditi-pandit Could you help me review this, thank you.

mbasmanova · 2023-12-12T11:07:47Z

@aditi-pandit Aditi, would you help review this PR?

aditi-pandit

Thanks @wypb. Did a quick round of review.

aditi-pandit · 2023-12-13T19:26:02Z

velox/functions/prestosql/DateTimeFunctions.h

      return;
    }

    auto timestamp = this->toTimestamp(timestampWithTimezone);
    auto dateTime = getDateTime(timestamp, nullptr);
    adjustDateTime(dateTime, unit);
+    auto tzID = unpackZoneKeyId(timestampWithTimezone);


Nit : Can this local variable be inlined in line 870 ?

aditi-pandit · 2023-12-13T19:32:33Z

velox/functions/prestosql/types/TimestampWithTimeZoneType.h

@@ -125,4 +124,18 @@ class TimestampWithTimeZoneTypeFactories : public CustomTypeFactories {

 void registerTimestampWithTimeZoneType();

+static constexpr int32_t TIME_ZONE_MASK = 0xFFF;


The velox convention is "Use kPascalCase for static constants and enumerators" So kTimezoneMask kMillisShift is better.

velox/functions/prestosql/types/TimestampWithTimeZoneType.h

aditi-pandit · 2023-12-13T21:08:53Z

velox/functions/prestosql/DateTimeFunctions.h

-    const auto milliseconds = *timestampWithTimezone.template at<0>();
+    auto milliseconds = unpackMillisUtc(timestampWithTimezone);
+    auto tzID = unpackZoneKeyId(timestampWithTimezone);
+
    Timestamp timestamp = Timestamp::fromMillis(milliseconds);


No need for the local variables. They are used only once and can be inlined.

aditi-pandit · 2023-12-13T21:09:25Z

velox/functions/prestosql/DateTimeFunctions.h

-    auto timezone =
-        util::getTimeZoneName(*timestampWithTimezone.template at<1>());
+    auto tzID = unpackZoneKeyId(timestampWithTimezone);
+    auto timezone = util::getTimeZoneName(tzID);


Inline tzId variable use.

aditi-pandit · 2023-12-13T21:10:06Z

velox/functions/prestosql/DateTimeFunctions.h

-      result.template get_writer_at<0>() = utcTimestamp.getSeconds() * 1000;
-      result.template get_writer_at<1>() =
-          *timestampWithTimezone.template at<1>();
+      auto milliseconds = unpackMillisUtc(timestampWithTimezone);


Nit : Inline local variables used only once.

aditi-pandit · 2023-12-13T21:10:39Z

velox/functions/prestosql/FromUnixTime.cpp

-    auto timestamps = BaseVector::create(BIGINT(), size, pool);
-    auto* rawTimestamps =
-        timestamps->asFlatVector<int64_t>()->mutableRawValues();
+    auto timestamps = AlignedBuffer::allocate<int64_t>(size, pool, false);


Nice refactor.

aditi-pandit · 2023-12-13T21:13:55Z

velox/functions/prestosql/types/TimestampWithTimeZoneType.cpp

      flatResult.setNull(row, true);
    } else {
-      Timestamp ts = Timestamp::fromMillis(timestampVector->valueAt(row));
+      auto timestampWithTimezone =
+          input.as<SimpleVector<int64_t>>()->valueAt(row);


input.as<SimpleVector<int64_t>> can be made a local variable outside of the applyToSelectedNoThrow to avoid the casting per row.

aditi-pandit · 2023-12-13T21:19:23Z

velox/functions/prestosql/aggregates/PrestoHasher.cpp

+    applyHashFunction(rows, *vector_.get(), hashes, [&](auto row) {
+      auto timestampWithTimeZone = vector_->valueAt<int64_t>(row);
+      auto timestamp = unpackMillisUtc(timestampWithTimeZone);
+      return hashInteger(timestamp);


timestampWithTimeZone and timestamp can both be inlined.

wypb · 2023-12-14T03:31:23Z

Hi @aditi-pandit Thank you for your review, I have modified the corresponding code according to the review comments. If you have time, please help to review it again. Thank you.

wypb · 2024-02-21T01:35:24Z

Hi @kagamiori wei, Thank you for your reply. I'll do it today.

facebook-github-bot · 2024-02-21T03:25:21Z

@kagamiori has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

spershin

@wypb

Thanks for working on this!
A added a couple of nits and a question.

spershin · 2024-02-21T18:54:29Z

velox/functions/prestosql/aggregates/PrestoHasher.cpp

@@ -135,6 +135,11 @@ FOLLY_ALWAYS_INLINE void PrestoHasher::hash<TypeKind::BIGINT>(
      // returns the corresponding value directly.
      return vector_->valueAt<int64_t>(row);
    });
+  } else if (isTimestampWithTimeZoneType(vector_->base()->type())) {
+    // Hash only timestamp value.


I understand that this is how it was before, but why do we ignore TZ part?

Hi @spershin, I believe it was because Presto Java also ignores TZ part and we followed that behavior (https://github.com/prestodb/presto/blob/36575b0ca7d61a0ecf0be8eac7efc0b2b0d3ec85/presto-common/src/main/java/com/facebook/presto/common/type/TimestampWithTimeZoneType.java#L51-L54). I found a relevant discussion here: prestodb/presto#17274.

spershin · 2024-02-21T18:55:00Z

velox/functions/prestosql/types/TimestampWithTimeZoneType.h

-  TimestampWithTimeZoneType()
-      : RowType({"timestamp", "timezone"}, {BIGINT(), SMALLINT()}) {}
+class TimestampWithTimeZoneType : public BigintType {
+  TimestampWithTimeZoneType() {}


Lint recommends

TimestampWithTimeZoneType() = default;

Already fixed.

spershin · 2024-02-21T18:55:55Z

velox/functions/prestosql/types/TimestampWithTimeZoneType.h

+static constexpr int32_t kTimezoneMask = 0xFFF;
+static constexpr int32_t kMillisShift = 12;


nit
static should not be used for globals in a header.
For these two constexpr would suffice.

Already fixed.

facebook-github-bot · 2024-02-22T02:19:36Z

@kagamiori has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

kagamiori · 2024-02-22T23:26:23Z

Hi @wypb, the change to Base64Test.timestampWithTimezone has just been merged internally. Could you rebase this PR onto the latest main one more time? Thanks!

There is a conflict with PrestoSerializer.cpp by the way.

wypb · 2024-02-23T03:41:35Z

Hi @kagamiori I have synchronized the latest code, and the relevant UT seems to have passed. PTAL, thank you.

facebook-github-bot · 2024-02-23T04:24:51Z

@kagamiori has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

majetideepak · 2024-02-28T05:21:31Z

I see that this landed yesterday here 6fdaff7
Should we close this PR?

kagamiori · 2024-02-28T21:09:11Z

I see that this landed yesterday here 6fdaff7 Should we close this PR?

Yeah, not sure why this PR was not auto-closed. I'm closing it now.

kagamiori · 2024-03-07T01:36:51Z

Hi @wypb, are you going to make a follow-up PR of prestodb/presto#21974 to remove the unused code for the old TimestampWithTimezone type and re-enable the unit test?

wypb · 2024-03-07T04:11:26Z

@kagamiori I will enable it today.

Summary: TIMESTAMP_WITH_TIME_ZONE has been converted to a primitive type in #7480. Update the documentation to match. Pull Request resolved: #9042 Reviewed By: Yuhta Differential Revision: D54885988 Pulled By: mbasmanova fbshipit-source-id: 76a3a8cafbaf812ff7573816b57b1d1cde099471

…r#9042) Summary: TIMESTAMP_WITH_TIME_ZONE has been converted to a primitive type in facebookincubator#7480. Update the documentation to match. Pull Request resolved: facebookincubator#9042 Reviewed By: Yuhta Differential Revision: D54885988 Pulled By: mbasmanova fbshipit-source-id: 76a3a8cafbaf812ff7573816b57b1d1cde099471

mbasmanova · 2024-06-27T13:41:04Z

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

wypb marked this pull request as draft November 9, 2023 03:44

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Nov 9, 2023

wypb force-pushed the timestamp_with_time_zone branch 4 times, most recently from 2c08673 to 7d2c503 Compare November 9, 2023 05:36

wypb marked this pull request as ready for review November 9, 2023 06:45

mbasmanova requested review from kagamiori and aditi-pandit November 9, 2023 12:14

majetideepak reviewed Nov 9, 2023

View reviewed changes

velox/functions/prestosql/types/TimestampWithTimeZoneType.h Show resolved Hide resolved

wypb mentioned this pull request Nov 14, 2023

Add TIME/TIME WITH TIME ZONE type #7212

Closed

wypb force-pushed the timestamp_with_time_zone branch from 10d44a7 to 17ad5d7 Compare November 16, 2023 02:45

aditi-pandit reviewed Dec 13, 2023

View reviewed changes

wypb force-pushed the timestamp_with_time_zone branch 2 times, most recently from 397af42 to 3eb9510 Compare December 14, 2023 03:23

wypb force-pushed the timestamp_with_time_zone branch 5 times, most recently from e3796dd to c1f7dff Compare December 14, 2023 11:58

wypb force-pushed the timestamp_with_time_zone branch 2 times, most recently from 048e21a to 1533794 Compare February 21, 2024 02:35

wypb mentioned this pull request Feb 21, 2024

[native] Disable timestampWithTimezone ut in Base64Test prestodb/presto#21974

Merged

wypb force-pushed the timestamp_with_time_zone branch 2 times, most recently from e35564a to 93819aa Compare February 21, 2024 05:56

spershin approved these changes Feb 21, 2024

View reviewed changes

wypb force-pushed the timestamp_with_time_zone branch 2 times, most recently from 31be7da to 4e02577 Compare February 22, 2024 00:53

wypb force-pushed the timestamp_with_time_zone branch from 980eb7b to 02b512b Compare February 23, 2024 00:45

Convert TIMESTAMP_WITH_TIME_ZONE to primitive type

5caa663

wypb force-pushed the timestamp_with_time_zone branch from 7b961af to 5caa663 Compare February 23, 2024 00:52

majetideepak mentioned this pull request Feb 28, 2024

Add to_iso8601(TimestampWithTimezone) Presto function #8550

Closed

kagamiori mentioned this pull request Feb 28, 2024

Fix timestamp with timezone comparison #8838

Closed

kagamiori closed this Feb 28, 2024

wypb deleted the timestamp_with_time_zone branch February 28, 2024 22:51

This was referenced Mar 7, 2024

[native] Update timestampWithTimezone unit test in Base64Test prestodb/presto#22107

Merged

Fix documentation for TIMESTAMP_WITH_TIME_ZONE type #9042

Closed

		@@ -125,4 +124,18 @@ class TimestampWithTimeZoneTypeFactories : public CustomTypeFactories {

		void registerTimestampWithTimeZoneType();

		static constexpr int32_t TIME_ZONE_MASK = 0xFFF;

		static constexpr int32_t kTimezoneMask = 0xFFF;
		static constexpr int32_t kMillisShift = 12;

Convert TIMESTAMP_WITH_TIME_ZONE to primitive type #7480

Convert TIMESTAMP_WITH_TIME_ZONE to primitive type #7480

Conversation

wypb commented Nov 9, 2023 • edited Loading

netlify bot commented Nov 9, 2023 • edited Loading

✅ Deploy Preview for meta-velox canceled.

majetideepak commented Nov 9, 2023 • edited Loading

wypb commented Nov 10, 2023

majetideepak commented Nov 10, 2023

wypb commented Nov 16, 2023

wypb commented Nov 22, 2023

wypb commented Nov 28, 2023

wypb commented Dec 12, 2023

mbasmanova commented Dec 12, 2023

aditi-pandit left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wypb commented Dec 14, 2023 • edited Loading

wypb commented Feb 21, 2024

facebook-github-bot commented Feb 21, 2024

spershin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Feb 22, 2024

kagamiori commented Feb 22, 2024 • edited Loading

wypb commented Feb 23, 2024

facebook-github-bot commented Feb 23, 2024

majetideepak commented Feb 28, 2024

kagamiori commented Feb 28, 2024 • edited Loading

kagamiori commented Mar 7, 2024

wypb commented Mar 7, 2024

mbasmanova commented Jun 27, 2024 • edited Loading

wypb commented Nov 9, 2023 •

edited

Loading

netlify bot commented Nov 9, 2023 •

edited

Loading

majetideepak commented Nov 9, 2023 •

edited

Loading

wypb commented Dec 14, 2023 •

edited

Loading

kagamiori commented Feb 22, 2024 •

edited

Loading

kagamiori commented Feb 28, 2024 •

edited

Loading

mbasmanova commented Jun 27, 2024 •

edited

Loading