Return Status for several utility datetime functions #8918

marin-ma · 2024-02-29T08:28:08Z

Modify utility functions daysSinceEpochFromDate, daysSinceEpochFromDayOfYear and daysSinceEpochFromWeekDate to return Status code. These functions are likely to be frequently used in sparksql functions. As such, they could achieve better performance without throwing exceptions.

marin-ma · 2024-02-29T08:28:25Z

This PR is based on some discussions here: #8812 (comment)

netlify · 2024-02-29T08:28:26Z

✅ Deploy Preview for meta-velox canceled.

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

marin-ma · 2024-03-01T01:35:47Z

@rui-mo Please kindly review. Thanks!

rui-mo

Thanks!

rui-mo · 2024-03-01T04:04:13Z

velox/type/TimestampConversion.cpp

-    daysSinceEpoch = daysSinceEpochFromDate(year, 1, 1);
+    if (!daysSinceEpochFromDate(year, 1, 1, daysSinceEpoch).ok()) {
+      return false;
+    }


Maybe simplified with VELOX_RETURN_IF?

Looks like it cannot. VELOX_RETURN_IF returns status code.

rui-mo · 2024-03-01T04:06:01Z

velox/type/TimestampConversion.cpp

@@ -327,8 +335,7 @@ bool tryParseDateString(
    }
  }

-  daysSinceEpoch = daysSinceEpochFromDate(year, month, day);
-  return true;
+  return daysSinceEpochFromDate(year, month, day, daysSinceEpoch).ok();


Do we need to check if daysSinceEpochFromDate returns ok here?

return daysSinceEpochFromDate(year, month, day, daysSinceEpoch).ok();

It will return true when status is ok, otherwise return false.

marin-ma · 2024-03-04T01:01:13Z

@mbasmanova Could you help to review? Thanks!

mbasmanova

I'm surprised there are not unit test changes. The APIs in TimestampConversion.h have changed, but there are not test changes. Are we missing test coverage for these APIs? If so, let's add it.

marin-ma · 2024-03-05T01:10:13Z

velox/type/TimestampConversion.cpp

@@ -528,10 +535,21 @@ int32_t getMaxDayOfMonth(int32_t year, int32_t month) {
 }

 int64_t daysSinceEpochFromDate(int32_t year, int32_t month, int32_t day) {
+  int64_t daysSinceEpoch;
+  auto status = daysSinceEpochFromDate(year, month, day, daysSinceEpoch);


@mbasmanova int64_t daysSinceEpochFromDate is preserved here and the unit tests still use this API. Should I remove it?

@marin-ma Is daysSinceEpochFromDate used only in unit tests? If so, then it is better to remove.

@mbasmanova The previous one is also in use by non-test code. e.g. in sparksql function call:

velox/velox/functions/sparksql/DateTimeFunctions.h

Lines 315 to 334 in b11aeeb

template <typename T>

struct MakeDateFunction {

VELOX_DEFINE_FUNCTION_TYPES(T);

FOLLY_ALWAYS_INLINE void call(

out_type<Date>& result,

const int32_t year,

const int32_t month,

const int32_t day) {

auto daysSinceEpoch = util::daysSinceEpochFromDate(year, month, day);

VELOX_USER_CHECK_EQ(

daysSinceEpoch,

(int32_t)daysSinceEpoch,

"Integer overflow in make_date({}, {}, {})",

year,

month,

day);

result = daysSinceEpoch;

}

};

@marin-ma I see. Looks like we have inconsistent APIs. Some provide 2 versions: one that returns Status and doesn't throw and another that throws. While other APIs provide only non-throwing version. Would it make sense to align all these APIs to provide only non-throwing behavior?

marin-ma · 2024-03-06T01:05:06Z

@mbasmanova The previous APIs that throws have been removed. Could you help to review again? Thanks!

mbasmanova

@marin-ma Looks good to me. Thanks.

mbasmanova · 2024-03-06T01:08:08Z

velox/type/tests/TimestampConversionTest.cpp

+            util::daysSinceEpochFromDate(year, month, day, daysSinceEpoch);
+        EXPECT_TRUE(status.isUserError());
+      };
+  EXPECT_NO_THROW(testDaysSinceEpochFromDateInvalid(1970, 1, -1));


Would it be possible to verify error message as well?

Updated in the latest commit.

facebook-github-bot · 2024-03-06T01:11:00Z

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

marin-ma · 2024-03-06T03:53:49Z

@mbasmanova There are internal linter and test failures. Could you provide the log?

https://github.com/facebookincubator/velox/runs/22322577035

facebook-github-bot · 2024-03-06T10:42:26Z

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

facebook-github-bot · 2024-03-06T13:55:17Z

@mbasmanova merged this pull request in d54b76a.

…or#8918) Summary: Modify utility functions `daysSinceEpochFromDate`, `daysSinceEpochFromDayOfYear` and `daysSinceEpochFromWeekDate` to return Status code. These functions are likely to be frequently used in sparksql functions. As such, they could achieve better performance without throwing exceptions. Pull Request resolved: facebookincubator#8918 Reviewed By: pedroerp Differential Revision: D54566326 Pulled By: mbasmanova fbshipit-source-id: d7d35a216deae01f7a5f07c96803deb0975689a1

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 Feb 29, 2024

rui-mo reviewed Mar 1, 2024

View reviewed changes

mbasmanova reviewed Mar 4, 2024

View reviewed changes

marin-ma commented Mar 5, 2024

View reviewed changes

marin-ma force-pushed the datetime-status branch from 901ab9a to 9d8e795 Compare March 5, 2024 15:01

mbasmanova approved these changes Mar 6, 2024

View reviewed changes

marin-ma added 3 commits March 6, 2024 11:54

return status for serveral utility datetime functions

8d9c38a

unify API

595f577

check error message

d844691

marin-ma force-pushed the datetime-status branch from 8cfa2d0 to d844691 Compare March 6, 2024 03:54

facebook-github-bot closed this in d54b76a Mar 6, 2024

facebook-github-bot added the Merged label Mar 6, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Return Status for several utility datetime functions #8918

Return Status for several utility datetime functions #8918

marin-ma commented Feb 29, 2024

marin-ma commented Feb 29, 2024

netlify bot commented Feb 29, 2024 •

edited

Loading

marin-ma commented Mar 1, 2024

rui-mo left a comment

rui-mo Mar 1, 2024

marin-ma Mar 1, 2024

rui-mo Mar 1, 2024

marin-ma Mar 1, 2024

marin-ma commented Mar 4, 2024

mbasmanova left a comment

marin-ma Mar 5, 2024

mbasmanova Mar 5, 2024

marin-ma Mar 5, 2024

mbasmanova Mar 5, 2024

marin-ma commented Mar 6, 2024

mbasmanova left a comment

mbasmanova Mar 6, 2024

marin-ma Mar 6, 2024

facebook-github-bot commented Mar 6, 2024

marin-ma commented Mar 6, 2024 •

edited

Loading

facebook-github-bot commented Mar 6, 2024

facebook-github-bot commented Mar 6, 2024

	template <typename T>
	struct MakeDateFunction {
	VELOX_DEFINE_FUNCTION_TYPES(T);

	FOLLY_ALWAYS_INLINE void call(
	out_type<Date>& result,
	const int32_t year,
	const int32_t month,
	const int32_t day) {
	auto daysSinceEpoch = util::daysSinceEpochFromDate(year, month, day);
	VELOX_USER_CHECK_EQ(
	daysSinceEpoch,
	(int32_t)daysSinceEpoch,
	"Integer overflow in make_date({}, {}, {})",
	year,
	month,
	day);
	result = daysSinceEpoch;
	}
	};

Return Status for several utility datetime functions #8918

Return Status for several utility datetime functions #8918

Conversation

marin-ma commented Feb 29, 2024

marin-ma commented Feb 29, 2024

netlify bot commented Feb 29, 2024 • edited Loading

✅ Deploy Preview for meta-velox canceled.

marin-ma commented Mar 1, 2024

rui-mo 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

marin-ma commented Mar 4, 2024

mbasmanova 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

marin-ma commented Mar 6, 2024

mbasmanova left a 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 Mar 6, 2024

marin-ma commented Mar 6, 2024 • edited Loading

facebook-github-bot commented Mar 6, 2024

facebook-github-bot commented Mar 6, 2024

netlify bot commented Feb 29, 2024 •

edited

Loading

marin-ma commented Mar 6, 2024 •

edited

Loading