Add Spark date_trunc function #11340

zml1206 · 2024-10-24T11:17:39Z

Add Spark date_trunc function.
Also refactor presto date_trunc function for reuse:
Move class DateTimeUnit to DateTimeFormatter.h.
Extend funciton fromDateTimeUnitString, which can be used with presto and spark, and move to TimeUtils.h.
Move adjustDateTime and adjustEpoch to TimeUtils.h.
Extract new function dateTrunc.

netlify · 2024-10-24T11:17:55Z

✅ Deploy Preview for meta-velox canceled.

Name	Link
🔨 Latest commit	`d544dde`
🔍 Latest deploy log	https://app.netlify.com/sites/meta-velox/deploys/672c3cd0fe18ba000853d699

rui-mo

Thanks. The key idea is that by moving some of the methods to the lib for reuse, we can reduce the code duplication.

velox/docs/functions/spark/datetime.rst

velox/functions/sparksql/DateTimeFunctions.h

rui-mo

Thanks for refactoring.

velox/functions/sparksql/DateTimeFunctions.h

rui-mo · 2024-10-29T06:18:48Z

velox/functions/lib/TimeUtils.h

@@ -123,4 +126,155 @@ struct InitSessionTimezone {
    timeZone_ = getTimeZoneFromConfig(config);
  }
 };
+
+FOLLY_ALWAYS_INLINE std::optional<DateTimeUnit> fromDateTimeUnitString(


Would you please add some comments for the new API? If there are unit tests for them, please move them into the lib; if not, we will need to add them.

And maybe mention it in the currently empty PR description/commit message.

jinchengchenghh

Thanks! Some minors. Could you also clarify what you have changed in the PR description?

jinchengchenghh · 2024-10-31T06:44:17Z

velox/docs/functions/spark/datetime.rst

+
+.. spark:function:: date_trunc(fmt, ts) -> timestamp
+
+    Returns timestamp ts truncated to the unit specified by the format model fmt.


Returns timestamp ts -> Returns timestamp that ts, by the format model fmt

jinchengchenghh · 2024-10-31T06:45:28Z

velox/docs/functions/spark/datetime.rst

+    Returns null if ``fmt`` is invalid.
+    ``fmt`` is case insensitive and must be one of the following:
+        * "YEAR", "YYYY", "YY" - truncate to the first date of the year that the ts falls in, the time part will be zero out
+        * "QUARTER" - truncate to the first date of the quarter that the ts falls in, the time part will be zero out


that the ts falls, please quote all the ts, thanks!

jinchengchenghh · 2024-10-31T06:56:09Z

velox/functions/lib/TimeUtils.h

+    bool allowAbbreviated = false) {
+  static const StringView kMicrosecond("microsecond");
+  static const StringView kMillisecond("millisecond");
+  static const StringView kSecond("second");


All this fields are used only once, do we need to set them? Or we could use the string directly in if condition? CC @rui-mo
For example, https://github.com/facebookincubator/velox/blob/main/velox/common/base/StatsReporter.h#L70

This is the original presto function code. Now it is just extracted to lib for reuse. Do we need to change it? @jinchengchenghh @rui-mo

Can we remove those defined in DateTimeFormatter then? And +1 for using string directly because these variables are used only once.

jinchengchenghh · 2024-10-31T07:00:20Z

velox/functions/lib/TimeUtils.h

+        }
+
+        // Calculate the correct day of the month based on the number of days
+        // in the adjusted month


Add . in the end.

jinchengchenghh · 2024-10-31T07:00:39Z

velox/functions/lib/TimeUtils.h

+            31, 28, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31};
+        int daysInPrevMonth = daysInMonth[dateTime.tm_mon];
+
+        // Adjust for leap year if February


jinchengchenghh · 2024-10-31T07:01:30Z

velox/functions/lib/TimeUtils.h

+             (dateTime.tm_year + 1900) % 400 == 0)) {
+          daysInPrevMonth = 29;
+        }
+        // Set to the correct day in the previous month


jinchengchenghh · 2024-10-31T07:06:35Z

velox/functions/lib/TimeUtils.h

+
+// Returns timestamp truncated to the unit specified by the format.
+FOLLY_ALWAYS_INLINE Timestamp dateTrunc(
+    const DateTimeUnit unit,


Don't add the const to enum class, you could consider it as integer like.
https://stackoverflow.com/questions/44749658/c-is-it-better-to-pass-an-enum-class-as-value-or-const-reference

@jinchengchenghh Thanks for review. It seems to be used this way in velox,

velox/velox/functions/prestosql/DateTimeFunctions.h

Line 929 in 1e7a82f

inline bool isTimeUnit(const DateTimeUnit unit) {

No need to add const when passing by value.

jinchengchenghh · 2024-10-31T07:16:04Z

velox/functions/sparksql/DateTimeFunctions.h

+struct DateTruncFunction {
+  VELOX_DEFINE_FUNCTION_TYPES(T);
+
+  const tz::TimeZone* timeZone_ = nullptr;


Please move the class member to the last of the struct as FromUnixtimeFunction.
And move it to private scope

jinchengchenghh · 2024-10-31T07:17:49Z

velox/functions/sparksql/DateTimeFunctions.h

+    if (!unitOption.has_value()) {
+      return false;
+    }
+    DateTimeUnit unit = unitOption.value();


const DateTimeUnit unit. I would prefer to drop this initialization since it is used only once.

rui-mo

Thanks.

velox/docs/functions/spark/datetime.rst

rui-mo · 2024-11-04T07:25:49Z

velox/functions/lib/TimeUtils.h

+    bool allowAbbreviated = false) {
+  static const StringView kMicrosecond("microsecond");
+  static const StringView kMillisecond("millisecond");
+  static const StringView kSecond("second");


Can we remove those defined in DateTimeFormatter then? And +1 for using string directly because these variables are used only once.

velox/functions/lib/TimeUtils.h

rui-mo · 2024-11-04T08:37:19Z

velox/functions/lib/TimeUtils.h

+
+// Returns timestamp truncated to the unit specified by the format.
+FOLLY_ALWAYS_INLINE Timestamp dateTrunc(
+    const DateTimeUnit unit,


No need to add const when passing by value.

velox/functions/lib/TimeUtils.h

fix style fix fix fix fix fix fix

fix style fix fix fix fix

update fix fix

jinchengchenghh · 2024-11-05T02:50:56Z

velox/functions/sparksql/Register.cpp

@@ -518,6 +518,8 @@ void registerFunctions(const std::string& prefix) {
      Varchar,
      Varchar,
      Varchar>({prefix + "mask"});
+  registerFunction<DateTruncFunction, Timestamp, Varchar, Timestamp>(


Please move the function registration after the date time functions https://github.com/facebookincubator/velox/pull/11340/files#diff-19962809ab2250cbf3c99e92b7ba14ae89e2f26ae5e646db3c2b31e67af16e8dL450

Moved, thanks.

jinchengchenghh · 2024-11-05T05:19:52Z

velox/docs/functions/spark/datetime.rst

+        * "MICROSECOND" - everything remains
+
+    Returns timestamp ``ts`` truncated to the unit specified by the format model ``fmt``.
+    Returns null if ``fmt`` is invalid. ::


What if ts is null?

return null.

https://github.com/facebookincubator/velox/pull/11340/files#diff-f7042b580cdd92f6b4ba8b77183933cbb460c88a6deed81f767a35bb4d2cc40eR1112

jinchengchenghh · 2024-11-05T05:53:29Z

velox/docs/functions/spark/datetime.rst

+        * "MILLISECOND" - zero out the microseconds
+        * "MICROSECOND" - everything remains
+
+    Returns timestamp ``ts`` truncated to the unit specified by the format model ``fmt``.


Please move the function description before argument description L56.

jinchengchenghh · 2024-11-05T06:05:11Z

velox/functions/lib/tests/TimeUtilsTest.cpp

+  ASSERT_EQ(std::nullopt, fromDateTimeUnitString("yyyy", false));
+  ASSERT_EQ(std::nullopt, fromDateTimeUnitString("yy", false));
+
+  ASSERT_EQ(


This is valid too.

ASSERT_EQ( DateTimeUnit::kMillisecond, fromDateTimeUnitString("millisecond", false));

And other public functions also need tests. I'm open for this point since the function test should have cover the function you moved, but it is better to add it.

jinchengchenghh · 2024-11-05T06:06:12Z

velox/functions/lib/tests/TimeUtilsTest.cpp

+  ASSERT_EQ(
+      std::optional(DateTimeUnit::kYear),
+      fromDateTimeUnitString("yyyy", false, true, true));
+  ASSERT_EQ(


Test invalid fmt by VELOX_ASSERT_THROW

update update

rui-mo

Thanks. Added some comments.

rui-mo · 2024-11-06T06:47:12Z

velox/docs/functions/spark/datetime.rst

+        * "MINUTE"- zero out the second with fraction part
+        * "SECOND" - zero out the second fraction part
+        * "MILLISECOND" - zero out the microseconds
+        * "MICROSECOND" - everything remains. ::


The example section does not render correctly. You could add the '::' as below to fix it.

* "MICROSECOND" - everything remains. :: SELECT date_trunc('YEAR', '2015-03-05T09:32:05.359'); -- 2015-01-01 00:00:00

rui-mo · 2024-11-06T06:48:54Z

velox/docs/functions/spark/datetime.rst

+        SELECT date_trunc('HOUR', '2015-03-05T09:32:05.359'); -- 2015-03-05 09:00:00
+        SELECT date_trunc('MINUTE', '2015-03-05T09:32:05.359'); -- 2015-03-05 09:32:00
+        SELECT date_trunc('SECOND', '2015-03-05T09:32:05.359'); -- 2015-03-05 09:32:05
+        SELECT date_trunc('MILLISECOND', '2015-03-05T09:32:05.123456'); -- 2015-03-05 09:32:05.123456


The microseconds are not zeroed out. Is this example correct?

rui-mo · 2024-11-06T06:49:18Z

velox/docs/functions/spark/datetime.rst

+        SELECT date_trunc('MINUTE', '2015-03-05T09:32:05.359'); -- 2015-03-05 09:32:00
+        SELECT date_trunc('SECOND', '2015-03-05T09:32:05.359'); -- 2015-03-05 09:32:05
+        SELECT date_trunc('MILLISECOND', '2015-03-05T09:32:05.123456'); -- 2015-03-05 09:32:05.123456
+        SELECT date_trunc('MILLISECOND', '2015-03-05T09:32:05..123456'); -- 2015-01-01 09:32:05.123456


Duplicate for millisecond.

rui-mo · 2024-11-06T06:50:22Z

velox/functions/lib/DateTimeFormatter.h

@@ -118,6 +118,19 @@ enum class DateTimeFormatSpecifier : uint8_t {
  WEEK_OF_MONTH = 24
 };

+enum class DateTimeUnit {


Could be removed?

You mean remove class DateTimeUnit?

rui-mo · 2024-11-06T06:51:50Z

velox/functions/lib/TimeUtils.h

+FOLLY_ALWAYS_INLINE std::optional<DateTimeUnit> fromDateTimeUnitString(
+    const StringView& unitString,
+    bool throwIfInvalid,
+    bool allowMirco = false,


typo: Mirco -> Micro

rui-mo · 2024-11-06T06:58:05Z

velox/functions/lib/TimeUtils.h

+  if (seconds < 0 && seconds % intervalSeconds) {
+    s = s - 1;
+  }
+  int64_t truncedSeconds = s * intervalSeconds;


typo: trunced -> truncated

rui-mo · 2024-11-06T07:03:11Z

velox/functions/lib/TimeUtils.h

+
+/// For fixed interval like minute, hour and day,
+/// we can truncate date by a simple arithmetic expression:
+/// floor(seconds / intervalSeconds) * intervalSeconds.


Please avoid repeating the code in comments. Comment suggestion:

/// Returns timestamp with seconds adjusted to the nearest lower multiple of the
/// specified interval. If the given seconds is negative and not an exact
/// multiple of the interval, it adjusts further down.

rui-mo · 2024-11-06T07:03:22Z

velox/functions/lib/TimeUtils.h

+/// we can truncate date by a simple arithmetic expression:
+/// floor(seconds / intervalSeconds) * intervalSeconds.
+FOLLY_ALWAYS_INLINE Timestamp
+adjustEpoch(int64_t seconds, int64_t intervalSeconds) {


Ditto. Do we have unit test for it?

rui-mo · 2024-11-06T07:04:22Z

velox/functions/lib/TimeUtils.h

+}
+
+// Returns timestamp truncated to the specified unit.
+FOLLY_ALWAYS_INLINE Timestamp truncateTimestamp(


Ditto. Better to add unit test.

rui-mo · 2024-11-06T07:05:35Z

velox/functions/sparksql/DateTimeFunctions.h

+  FOLLY_ALWAYS_INLINE void initialize(
+      const std::vector<TypePtr>& /*inputTypes*/,
+      const core::QueryConfig& config,
+      const arg_type<Varchar>* format,


nit: format is not used either.

add ut

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 Oct 24, 2024

zml1206 mentioned this pull request Oct 24, 2024

Register re-usable Presto date_trunc functions for Spark #11304

Closed

rui-mo changed the title ~~add date_trunc spark function~~ Add Spark date_trunc function Oct 28, 2024

rui-mo reviewed Oct 28, 2024

View reviewed changes

zml1206 requested a review from rui-mo October 29, 2024 06:17

rui-mo reviewed Oct 29, 2024

View reviewed changes

zml1206 requested review from assignUser and majetideepak as code owners October 29, 2024 16:11

zml1206 requested a review from rui-mo October 30, 2024 07:08

jinchengchenghh reviewed Oct 31, 2024

View reviewed changes

rui-mo reviewed Nov 4, 2024

View reviewed changes

zml1206 added 7 commits November 5, 2024 09:15

add date_trunc spark function

ba850b2

fix style fix fix fix fix fix fix

update for reuse

41c6916

fix style fix fix fix fix

fix style

0e2f3ed

update

2a1bf1b

update fix fix

update

b3e692b

update

dbd8d7a

fix

8ab10ec

zml1206 force-pushed the add_date_trunc branch from 525fdaa to 8ab10ec Compare November 5, 2024 01:20

fix

bd444dd

jinchengchenghh reviewed Nov 5, 2024

View reviewed changes

update

9dfb9a6

jinchengchenghh reviewed Nov 5, 2024

View reviewed changes

update

bafd7d7

update update

rui-mo reviewed Nov 6, 2024

View reviewed changes

add ut

d544dde

add ut

zml1206 requested a review from rui-mo November 15, 2024 01:40


		.. spark:function:: date_trunc(fmt, ts) -> timestamp

		Returns timestamp ts truncated to the unit specified by the format model fmt.

Add Spark date_trunc function #11340

Are you sure you want to change the base?

Add Spark date_trunc function #11340

Conversation

zml1206 commented Oct 24, 2024 • edited Loading

netlify bot commented Oct 24, 2024 • edited Loading

✅ Deploy Preview for meta-velox canceled.

rui-mo left a comment

Choose a reason for hiding this comment

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

jinchengchenghh 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

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

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

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

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

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

zml1206 commented Oct 24, 2024 •

edited

Loading

netlify bot commented Oct 24, 2024 •

edited

Loading