Fix Spark WeekFunction on long years #10713

zml1206 · 2024-08-12T08:32:07Z

Similar to #10599, in Spark, a long year is any year ending on Thursday and any
leap year ending on Friday. Since the only difference between Presto and Spark
is on return type, this PR extracts the 'getWeek' function to functions/lib and
changed to use 'weeknum' from external/date library for the week calculation.

facebook-github-bot · 2024-08-12T08:32:13Z

Hi @zml1206!

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-08-12T08:32:23Z

✅ Deploy Preview for meta-velox canceled.

Name	Link
🔨 Latest commit	`027c9b1`
🔍 Latest deploy log	https://app.netlify.com/sites/meta-velox/deploys/66f5cdeba50ffc00086002e3

rui-mo

Thanks for the fix. Looks like the implementation is nearly the same with Presto's except for the return type (seeing #5941). To improve code maintainability, any chance to extract a template function into 'velox/functions/lib' and use it for both Presto and Spark?

pedroerp · 2024-08-12T15:27:57Z

I recently vendored another part of external/date library that let you calculate weeks of year. Could you just use it here instead? I provides the exact ISO semantic, which I guess is what Spark expects?

https://github.com/facebookincubator/velox/pull/10686/files

Cc: @kevinwilfong

rui-mo

Thanks. Added some comments.

velox/functions/lib/TimeUtils.h

velox/functions/prestosql/DateTimeFunctions.h

velox/functions/sparksql/tests/DateTimeFunctionsTest.cpp

velox/functions/sparksql/DateTimeFunctions.h

velox/type/Timestamp.h

zml1206 · 2024-08-15T07:49:55Z

I recently vendored another part of external/date library that let you calculate weeks of year. Could you just use it here instead? I provides the exact ISO semantic, which I guess is what Spark expects?

external/date has different Max and min year. I'm not sure if can change it to Joda datetime min and max.
Or use the original method in this pr, and then consider whether to use the external/date library uniformly later? @pedroerp @rui-mo

pedroerp · 2024-08-16T00:58:11Z

I recently vendored another part of external/date library that let you calculate weeks of year. Could you just use it here instead? I provides the exact ISO semantic, which I guess is what Spark expects?

external/date has different Max and min year. I'm not sure if can change it to Joda datetime min and max. Or use the original method in this pr, and then consider whether to use the external/date library uniformly later? @pedroerp @rui-mo

Fair enough, but I feel like even the narrower min/max provided by external/date should be enough to cover any realistic date ranges. IMHO the value of consolidating this logic is probably larger; we could also always look into relaxing the date restrictions in external/date (I think @gggrace14 did something similar in the past) if this becomes a must.

zml1206 · 2024-08-16T04:00:20Z

velox/functions/prestosql/tests/DateTimeFunctionsTest.cpp

@@ -488,7 +488,7 @@ TEST_F(DateTimeFunctionsTest, week) {
                    VELOX_USER_FAIL("{}", status.message());
                  });
    auto timestamp =
-        std::make_optional(Timestamp(ts.getSeconds() * 100'000'000, 0));
+        std::make_optional(Timestamp(ts.getSeconds() * 100'000, 0));


Change it avoid year range error.

zml1206 · 2024-08-19T02:32:53Z

Fair enough, but I feel like even the narrower min/max provided by external/date should be enough to cover any realistic date ranges. IMHO the value of consolidating this logic is probably larger; we could also always look into relaxing the date restrictions in external/date (I think @gggrace14 did something similar in the past) if this becomes a must.

Can you help take a look again, thanks @pedroerp

rui-mo · 2024-08-19T06:47:05Z

velox/functions/prestosql/tests/DateTimeFunctionsTest.cpp

+  EXPECT_EQ(33, weekTimestamp("T06:01:01"));
+  EXPECT_EQ(44, weekTimestamp("T06:59:59"));
+  EXPECT_EQ(47, weekTimestamp("T12:00:01"));
+  EXPECT_EQ(16, weekTimestamp("T12:59:59"));


@zml1206 Wondering why we are getting different results with this change.

Because the timestamp becomes 1/1000 of the original value.

As long as we have a way to verify externally that these are in fact the correct values for year 100k

velox/functions/sparksql/DateTimeFunctions.h

velox/functions/lib/TimeUtils.h

velox/functions/sparksql/DateTimeFunctions.h

zml1206 · 2024-08-28T01:59:16Z

Whether regression is accidental and has nothing to do with pr? @rui-mo

rui-mo · 2024-08-28T14:02:05Z

Whether regression is accidental and has nothing to do with pr? @rui-mo

We may need a Meta employee to help check the details.

rui-mo · 2024-08-28T14:05:54Z

velox/functions/sparksql/DateTimeFunctions.h

  FOLLY_ALWAYS_INLINE void call(int32_t& result, const arg_type<Date>& date) {
-    result = getWeek(getDateTime(date));
+    int64_t seconds = (int64_t)date * kSecondsInDay;
+    result = getWeek(Timestamp(seconds, 0), nullptr, false);


Wondering if overflow can happen when the date is very large, and what is the Spark's behavior when an overflow occurs? Thanks.

Sorry for the late reply, I'm on vacation for a few days. Spark will returns null.

pedroerp

A few small comments, but overall looks good.

pedroerp · 2024-09-09T20:26:46Z

velox/functions/lib/TimeUtils.h

@@ -92,6 +94,23 @@ FOLLY_ALWAYS_INLINE int32_t getDayOfYear(const std::tm& time) {
  return time.tm_yday + 1;
 }

+FOLLY_ALWAYS_INLINE unsigned getWeek(


nit: could we use something like uint32_t to make the type length more explicit?

pedroerp · 2024-09-09T20:30:06Z

velox/functions/prestosql/DateTimeFunctions.h

  }

  FOLLY_ALWAYS_INLINE void call(int64_t& result, const arg_type<Date>& date) {
-    result = getWeek(getDateTime(date));
+    int64_t seconds = (int64_t)date * kSecondsInDay;


seems like here we could add another factory method on Timestamp to encapsulate this logic, like:

result = getWeek(Timestamp::fromDate(date), nullptr, false);

Updated, thank you.

pedroerp · 2024-09-09T20:31:34Z

velox/functions/prestosql/tests/DateTimeFunctionsTest.cpp

+  EXPECT_EQ(33, weekTimestamp("T06:01:01"));
+  EXPECT_EQ(44, weekTimestamp("T06:59:59"));
+  EXPECT_EQ(47, weekTimestamp("T12:00:01"));
+  EXPECT_EQ(16, weekTimestamp("T12:59:59"));


As long as we have a way to verify externally that these are in fact the correct values for year 100k

pedroerp

Thank you

pedroerp · 2024-09-10T14:26:02Z

velox/type/Timestamp.h

@@ -117,6 +117,9 @@ struct Timestamp {
  /// and the number of nanoseconds.
  static Timestamp fromDaysAndNanos(int32_t days, int64_t nanos);

+  // date is since unix epoch.
+  static Timestamp fromDate(int32_t date);


Could you also add a quick comment explaining what date represents?

zml1206 · 2024-09-13T03:45:20Z

Failure CI seems unrelated to PR.

pedroerp · 2024-09-16T16:51:50Z

Thank you @zml1206 . Looks good to me.

zml1206 · 2024-09-18T06:09:17Z

Can it be merged? @pedroerp @rui-mo Thank you.

facebook-github-bot · 2024-09-23T17:52:07Z

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

pedroerp · 2024-09-23T18:02:55Z

@zml1206 looks like Spark fuzzer is failing. Could you check?

zml1206 · 2024-09-24T12:25:52Z

velox/expression/fuzzer/SparkExpressionFuzzerTest.cpp

@@ -69,6 +69,9 @@ int main(int argc, char** argv) {
      // timestamp_millis(bigint) can generate timestamps out of the supported
      // range that make other functions throw VeloxRuntimeErrors.
      "timestamp_millis(bigint) -> timestamp",
+      // week_of_year throws VeloxRuntimeError when the year is out of the
+      // supported range.
+      "week_of_year",


Similar to #10928.

zml1206 · 2024-09-24T12:31:07Z

@zml1206 looks like Spark fuzzer is failing. Could you check?

It seems to be caused by modifying the error type in #10993 and #10928. Range errors occasionally occur. Skip it first. @pedroerp

pedroerp · 2024-09-24T16:04:50Z

@zml1206 Skipping the function means we won't be testing it at all. @kagamiori what is the right way of leaving this enabled?

kagamiori · 2024-09-24T17:09:53Z

Hi

@zml1206 Skipping the function means we won't be testing it at all. @kagamiori what is the right way of leaving this enabled?

Hi @zml1206, do you have a link of the spark and expression fuzzer failure in CI because of the week_of_year function? I can take a look to see why it's not suppressed by #10993.

zml1206 · 2024-09-24T20:23:44Z

Hi

@zml1206 Skipping the function means we won't be testing it at all. @kagamiori what is the right way of leaving this enabled?

Hi @zml1206, do you have a link of the spark and expression fuzzer failure in CI because of the week_of_year function? I can take a look to see why it's not suppressed by #10993.

@kagamiori Thank you.
https://github.com/facebookincubator/velox/actions/runs/11005107512/job/30559109546?pr=10713

…hrown from one eval path Summary: Velox throws VeloxRuntimeError at a few places intentionally to fail the query instead of generating wrong results, e.g., when timestamp is outside the supported range. These VeloxRuntimeErrors caused failures in expression fuzzer, hence an attempt was made to tolerate such runtime errors in fuzzer so that fuzzer can still test the relevant functions (facebookincubator#10993). However, it turns out that that attempt was not enough. The reason is that VeloxRuntimeError doesn't work well with the existing mechanism of masking error by default nulls. So the expression fuzzer still occasionally fails because of only one of the common and simple evaluation path throwing the error. (If we later want to make those VeloxRuntimeError not suppressable by TRY but suppressable by the mask-error-by-default-null mechanism, we'll need to extend the EvalErrors struct with an additional buffer to track whether the error at an index can be suppressed by Try and check that in TryExpr.) This diff relaxes the fuzzer check to make fuzzer not fail when only one of the common and simple eval path throw the UNSUPPORTED_INPUT_UNCATCHABLE runtime error. This should unblock facebookincubator#10713. Differential Revision: D63421914

…hrown from one eval path (#11096) Summary: Pull Request resolved: #11096 Velox throws VeloxRuntimeError at a few places intentionally to fail the query instead of generating wrong results, e.g., when timestamp is outside the supported range. These VeloxRuntimeErrors caused failures in expression fuzzer, hence an attempt was made to tolerate such runtime errors in fuzzer so that fuzzer can still test the relevant functions (#10993). However, it turns out that that attempt was not enough. The reason is that VeloxRuntimeError doesn't work well with the existing mechanism of masking error by default nulls. So the expression fuzzer still occasionally fails because of only one of the common and simple evaluation path throwing the error. (If we later want to make those VeloxRuntimeError not suppressable by TRY but suppressable by the mask-error-by-default-null mechanism, we'll need to extend the EvalErrors struct with an additional buffer to track whether the error at an index can be suppressed by Try and check that in TryExpr.) This diff relaxes the fuzzer check to make fuzzer not fail when only one of the common and simple eval path throw the UNSUPPORTED_INPUT_UNCATCHABLE runtime error. This should unblock #10713. Reviewed By: kevinwilfong Differential Revision: D63421914 fbshipit-source-id: 54d01f7a5e7ea887018659d0eec8070fd0922b50

kagamiori · 2024-09-26T18:30:07Z

Hi @zml1206, the fix for the fuzzer test has been merged. Could you rebase the PR on top of the latest main and remove week_of_year from the skip-function list to see if there is still any failure? Thanks!

extract getWeek update fix fix fix fix DateTimeFunctionsTest.week fix update update update comment

zml1206 · 2024-09-27T01:06:25Z

Hi @zml1206, the fix for the fuzzer test has been merged. Could you rebase the PR on top of the latest main and remove week_of_year from the skip-function list to see if there is still any failure? Thanks!

It is resolved, thanks @kagamiori . cc @pedroerp

facebook-github-bot · 2024-10-01T06:19:00Z

@pedroerp merged this pull request in 5862a4d.

conbench-facebook · 2024-10-01T07:10:19Z

Conbench analyzed the 1 benchmark run on commit 5862a4dc.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

rui-mo reviewed Aug 12, 2024

View reviewed changes

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 Aug 12, 2024

zml1206 mentioned this pull request Aug 13, 2024

Result mismatch of weekOfYear apache/incubator-gluten#6784

Open

rui-mo reviewed Aug 14, 2024

View reviewed changes

rui-mo reviewed Aug 15, 2024

View reviewed changes

velox/functions/sparksql/DateTimeFunctions.h Outdated Show resolved Hide resolved

velox/type/Timestamp.h Outdated Show resolved Hide resolved

zml1206 commented Aug 16, 2024

View reviewed changes

rui-mo reviewed Aug 19, 2024

View reviewed changes

zml1206 requested a review from rui-mo August 21, 2024 08:28

rui-mo changed the title ~~Fix result mismatch of spark WeekFunciton~~ Fix Spark WeekFunciton on long years Aug 22, 2024

rui-mo reviewed Aug 22, 2024

View reviewed changes

velox/functions/lib/TimeUtils.h Outdated Show resolved Hide resolved

velox/functions/sparksql/DateTimeFunctions.h Outdated Show resolved Hide resolved

rui-mo reviewed Aug 28, 2024

View reviewed changes

ccat3z mentioned this pull request Aug 30, 2024

[VL] Result mismatch in date format week year apache/incubator-gluten#7069

Open

pedroerp reviewed Sep 9, 2024

View reviewed changes

pedroerp approved these changes Sep 10, 2024

View reviewed changes

pedroerp added the ready-to-merge PR that have been reviewed and are ready for merging. PRs with this tag notify the Velox Meta oncall label Sep 10, 2024

zml1206 force-pushed the fix_week branch from 9f8b29b to 60e428c Compare September 12, 2024 02:51

pedroerp changed the title ~~Fix Spark WeekFunciton on long years~~ Fix Spark WeekFunction on long years Sep 23, 2024

zml1206 force-pushed the fix_week branch 2 times, most recently from 87af294 to 54cad4b Compare September 24, 2024 12:20

zml1206 commented Sep 24, 2024

View reviewed changes

kagamiori mentioned this pull request Sep 25, 2024

Fix ExpressionVerifier to ignore UNSUPPORTED_INPUT_UNCATCHABLE only thrown from one eval path #11096

Closed

Fix result mismatch of spark WeekFunciton

027c9b1

extract getWeek update fix fix fix fix DateTimeFunctionsTest.week fix update update update comment

zml1206 force-pushed the fix_week branch from 54cad4b to 027c9b1 Compare September 26, 2024 21:11

facebook-github-bot closed this in 5862a4d Oct 1, 2024

facebook-github-bot added the Merged label Oct 1, 2024

Fix Spark WeekFunction on long years #10713

Fix Spark WeekFunction on long years #10713

Conversation

zml1206 commented Aug 12, 2024 • edited by rui-mo Loading

facebook-github-bot commented Aug 12, 2024

Action Required

Process

netlify bot commented Aug 12, 2024 • edited Loading

✅ Deploy Preview for meta-velox canceled.

rui-mo left a comment

Choose a reason for hiding this comment

pedroerp commented Aug 12, 2024 • edited Loading

rui-mo left a comment

Choose a reason for hiding this comment

zml1206 commented Aug 15, 2024 • edited Loading

pedroerp commented Aug 16, 2024 • edited Loading

Choose a reason for hiding this comment

zml1206 commented Aug 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zml1206 commented Aug 28, 2024

rui-mo commented Aug 28, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pedroerp 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

pedroerp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zml1206 commented Sep 13, 2024

pedroerp commented Sep 16, 2024

zml1206 commented Sep 18, 2024

facebook-github-bot commented Sep 23, 2024

pedroerp commented Sep 23, 2024

Choose a reason for hiding this comment

zml1206 commented Sep 24, 2024

pedroerp commented Sep 24, 2024

kagamiori commented Sep 24, 2024

zml1206 commented Sep 24, 2024

kagamiori commented Sep 26, 2024

zml1206 commented Sep 27, 2024

facebook-github-bot commented Oct 1, 2024

conbench-facebook bot commented Oct 1, 2024

zml1206 commented Aug 12, 2024 •

edited by rui-mo

Loading

netlify bot commented Aug 12, 2024 •

edited

Loading

pedroerp commented Aug 12, 2024 •

edited

Loading

zml1206 commented Aug 15, 2024 •

edited

Loading

pedroerp commented Aug 16, 2024 •

edited

Loading