Add allowOverflow flag to Timestamp::toTimezone #9836

NEUpanning · 2024-05-16T02:45:37Z

Allow integer overflow when converting timestamp to TimePoint.

Use in Spark function from_unixtime that allows overflow.

facebook-github-bot · 2024-05-16T02:45:42Z

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks!

netlify · 2024-05-16T02:45:53Z

✅ Deploy Preview for meta-velox canceled.

Name	Link
🔨 Latest commit	`5f712d3`
🔍 Latest deploy log	https://app.netlify.com/sites/meta-velox/deploys/664da6b3690ab600084f7453

facebook-github-bot · 2024-05-16T03:19:04Z

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!

NEUpanning · 2024-05-17T03:38:02Z

@PHILO-HE @rui-mo Could you please help to take a look? Thanks.

rui-mo

Thanks for the fix.

velox/functions/sparksql/tests/DateTimeFunctionsTest.cpp

rui-mo

Thanks. Looks good overall.

velox/type/Timestamp.h

velox/functions/sparksql/tests/DateTimeFunctionsTest.cpp

rui-mo

Thanks. Looks good % this question #9836 (comment).

rui-mo · 2024-05-17T09:23:45Z

@mbasmanova Would you like to take a review? Thanks.

PHILO-HE

Please revise the PR title. Assume this fix is not only required by from_unixtime.

My suggestion:
Fix time conversion overflow for `Timestamp::toTimezone`

mbasmanova · 2024-05-17T12:25:03Z

@rui-mo @PHILO-HE Rui, I'll take a look once your's and @PHILO-HE's comments are resolved, but would it be possible to update PR description to provide more context for this change?

NEUpanning · 2024-05-20T02:38:08Z

@rui-mo @PHILO-HE I've revised the PR title and added more context in PR description. Please take a look and provide your feedback.

rui-mo

Looks good to me. Thanks.

PHILO-HE

Looks good to me! @mbasmanova, please take a look. Thanks!

mbasmanova

@NEUpanning Some questions.

mbasmanova · 2024-05-21T02:08:09Z

velox/functions/sparksql/tests/DateTimeFunctionsTest.cpp

+#ifdef NDEBUG
+  // Integer overflow in the internal conversion from seconds to milliseconds.
+  EXPECT_EQ(
+      fromUnixTime(std::numeric_limits<int64_t>::max(), "yyyy-MM-dd HH:mm:ss"),


The example in the documentation shows a different result: https://facebookincubator.github.io/velox/functions/spark/datetime.html#from_unixtime

SELECT from_unixtime(9223372036854775807, "yyyy-MM-dd HH:mm:ss"); -- '1969-12-31 23:59:59'

Which one is the "right" one?

Both are right. The example in the documentation doesn't set query time zone configuration, but this test sets time zone to "Asia/Shanghai" , so the results are different.

Maybe we should add another example in document for query with time zone.

I added example in document : 59968ef

mbasmanova · 2024-05-21T02:09:00Z

velox/functions/sparksql/tests/DateTimeFunctionsTest.cpp

@@ -898,6 +898,15 @@ TEST_F(DateTimeFunctionsTest, fromUnixtime) {

  // 8 hours ahead UTC.
  setQueryTimeZone("Asia/Shanghai");
+// In debug mode, Timestamp constructor will throw exception if range check


Is there a deeper problem here that Timestamp cannot represent values representable in Spark? If not, then, perhaps there is some issue with the implementation where it attempts to create an "invalid" Timestamp.

Presto's Timestamp is stored in one 64-bit signed integer for milliseconds, but Spark's Timestamp is stored in 64-bit signed integer for seconds, so this test attempts to create an "invalid" Timestamp. Maybe we should remove the range check for Timestamp since it's also used by Spark now.

It would be nice to follow-up on that. CC: @rui-mo

I will work on it when i have time.

@NEUpanning Do we need to create an issue to track this follow-up?

@rui-mo Yes, i opened a issue : #9904

mbasmanova · 2024-05-22T06:39:49Z

velox/docs/functions/spark/datetime.rst

@@ -93,6 +93,10 @@ These functions support TIMESTAMP and DATE input types.
        SELECT from_unixtime(3600, 'yyyy'); -- '1970'
        SELECT from_unixtime(9223372036854775807, "yyyy-MM-dd HH:mm:ss");  -- '1969-12-31 23:59:59'

+    If we run the following query in the `Asia/Shanghai` time zone: ::


Would it be possible to include all 3 queries above, but in this time zone?

The same queries evaluated with `Asia/Shanghai` session time zone return timestamps shifted by 8 hours. SELECT ...

Sure. See 15e85ed

mbasmanova

Thanks.

facebook-github-bot · 2024-05-22T07:19:53Z

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

mbasmanova · 2024-05-22T07:28:59Z

@NEUpanning Would you rebase?

NEUpanning · 2024-05-22T08:05:19Z

@mbasmanova I've rebased to main branch and squashed commits.

facebook-github-bot · 2024-05-22T12:55:03Z

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

facebook-github-bot · 2024-05-22T17:05:08Z

@mbasmanova merged this pull request in 47fc3dd.

conbench-facebook · 2024-05-22T17:47:54Z

Conbench analyzed the 1 benchmark run on commit 47fc3dda.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

Summary: Allow integer overflow when converting timestamp to TimePoint. Use in Spark function from_unixtime that allows overflow. Fixes facebookincubator#9778 Pull Request resolved: facebookincubator#9836 Reviewed By: amitkdutta Differential Revision: D57659010 Pulled By: mbasmanova fbshipit-source-id: d60fd686c19dff02eebba1a0c0410056d189c6f5

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 May 16, 2024