feat: Add Spark date_trunc function #11340

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

Adds Spark date_trunc function. Presto date_trunc function was refactored for reuse:

Moves class DateTimeUnit to functions/lib/DateTimeFormatter.h.
Extends function fromDateTimeUnitString by adding params allowMicro and
allowAbbreviated for Spark, and moves to functions/lib/TimeUtils.h.
Moves adjustDateTime and adjustEpoch functions to functions/lib/TimeUtils.h.
Extracts new function truncateTimestamp to functions/lib/TimeUtils.h .

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

✅ Deploy Preview for meta-velox canceled.

Name	Link
🔨 Latest commit	`fe5df23`
🔍 Latest deploy log	https://app.netlify.com/sites/meta-velox/deploys/67cebc88602f700008b54d88

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

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

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.

velox/functions/lib/DateTimeFormatter.h

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.

velox/functions/lib/TimeUtils.h

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.

stale · 2025-02-14T02:37:53Z

This pull request has been automatically marked as stale because it has not had recent activity. If you'd still like this PR merged, please comment on the PR, make sure you've addressed reviewer comments, and rebase on the latest main. Thank you for your contributions!

rui-mo · 2025-02-14T21:33:15Z

@zml1206 Would you like to rebase this PR? Thanks.

zml1206 · 2025-02-17T01:24:22Z

@zml1206 Would you like to rebase this PR? Thanks.

Rebased, CI failures seems unrelated. @rui-mo

fix style fix fix fix fix fix fix update for reuse fix style fix fix fix fix fix style update update fix fix update update fix fix update update update update add ut add ut

rui-mo

Thanks! Looks good overall.

Extend funciton fromDateTimeUnitString

Would be elaborate how the function is extended in the PR description? Thanks.

velox/functions/lib/TimeUtils.h

velox/functions/sparksql/DateTimeFunctions.h

velox/functions/lib/TimeUtils.h

zml1206 · 2025-02-27T06:10:41Z

@rui-mo all comments resolved, can you help take a look again? thanks.

rui-mo

Thanks. Looks nice! Just added some nits.

velox/functions/lib/TimeUtils.h

velox/functions/sparksql/DateTimeFunctions.h

velox/docs/functions/spark/datetime.rst

rui-mo

Thanks. Just several nits.

rui-mo · 2025-03-10T08:50:50Z

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 null if ``fmt`` is invalid, microseconds and abbreviated unit string are allowed.


Comment suggestion:

Returns null if ``fmt`` is invalid. ``fmt`` as "MICROSECOND" and abbreviated unit string are allowed.

rui-mo · 2025-03-10T09:46:34Z

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 null if ``fmt`` is invalid, microseconds and abbreviated unit string are allowed.


nit: null -> NULL

rui-mo · 2025-03-10T09:47:14Z

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.123
+        SELECT date_trunc('MICROSECOND', '2015-03-05T09:32:05.123456'); -- 2015-03-05 09:32:05.123456


Would you provide an example of invalid fmt string?

rui-mo · 2025-03-10T09:51:23Z

velox/functions/lib/TimeUtils.cpp

+}
+
+Timestamp truncateTimestamp(
+    const Timestamp& timestamp,


Timestamp is fixed-width. Better to pass by value.

rui-mo · 2025-03-10T09:52:19Z

velox/functions/lib/TimeUtils.cpp

@@ -25,4 +25,179 @@ const folly::F14FastMap<std::string, int8_t> kDayOfWeekNames{
    {"tue", 5},      {"wed", 6},    {"thursday", 0}, {"friday", 1},
    {"saturday", 2}, {"sunday", 3}, {"monday", 4},   {"tuesday", 5},
    {"wednesday", 6}};
+
+std::optional<DateTimeUnit> fromDateTimeUnitString(
+    const StringView& unitString,


Ditto. Can pass by value.

rui-mo · 2025-03-10T09:54:39Z

velox/functions/sparksql/DateTimeFunctions.h

@@ -515,6 +515,40 @@ struct DateFromUnixDateFunction {
  }
 };

+/// Truncates a timestamp to a specified time unit. Return null if the
+/// format is invalid, microseconds and abbreviated unit string are allowed.


Return NULL if the format is invalid. Format as abbreviated unit string and "microseconds" are allowed.

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 force-pushed the add_date_trunc branch from 525fdaa to 8ab10ec Compare November 5, 2024 01:20

jinchengchenghh reviewed Nov 5, 2024

View reviewed changes

rui-mo reviewed Nov 6, 2024

View reviewed changes

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

stale bot added the stale label Feb 14, 2025

stale bot removed the stale label Feb 14, 2025

zml1206 force-pushed the add_date_trunc branch from d544dde to 67b53d7 Compare February 15, 2025 01:34

zml1206 changed the title ~~Add Spark date_trunc function~~ feat: Add Spark date_trunc function Feb 15, 2025

rui-mo mentioned this pull request Feb 17, 2025

feat: Add Spark date_trunc and trunc function #12352

Open

add date_trunc spark function

712b231

fix style fix fix fix fix fix fix update for reuse fix style fix fix fix fix fix style update update fix fix update update fix fix update update update update add ut add ut

zml1206 force-pushed the add_date_trunc branch from 67b53d7 to 712b231 Compare February 19, 2025 01:17

rui-mo reviewed Feb 24, 2025

View reviewed changes

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

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

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

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

update

be9c5c8

jinchengchenghh reviewed Feb 26, 2025

View reviewed changes

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

zml1206 added 2 commits February 26, 2025 19:26

fix style

0cc2c51

fix

da1f247

rui-mo approved these changes Mar 3, 2025

View reviewed changes

update

d5b5474

rui-mo approved these changes Mar 10, 2025

View reviewed changes

address comments

fe5df23


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

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

feat: Add Spark date_trunc function #11340

Are you sure you want to change the base?

feat: Add Spark date_trunc function #11340

Conversation

zml1206 commented Oct 24, 2024 • edited by rui-mo 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

stale bot commented Feb 14, 2025

rui-mo commented Feb 14, 2025

zml1206 commented Feb 17, 2025

rui-mo left a comment

Choose a reason for hiding this comment

zml1206 commented Feb 27, 2025

rui-mo left a comment • edited Loading

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

zml1206 commented Oct 24, 2024 •

edited by rui-mo

Loading

netlify bot commented Oct 24, 2024 •

edited

Loading

rui-mo left a comment •

edited

Loading