feat: Add Spark date_trunc function by zml1206 · Pull Request #11340 · facebookincubator/velox

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	`ca45de4`
🔍 Latest deploy log	https://app.netlify.com/sites/meta-velox/deploys/67ed4a46fe927a0008684fa5

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.

rui-mo

Thanks for refactoring.

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

  }
 };
+
+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

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

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

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

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

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

Add . in the end.

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

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

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

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

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

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

rui-mo

Thanks.

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

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

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

No need to add const when passing by value.

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

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

What if ts is null?

return null.

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

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

Please move the function description before argument description L56.

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

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

Test invalid fmt by VELOX_ASSERT_THROW

rui-mo

Thanks. Added some comments.

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

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

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

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

Duplicate for millisecond.

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

typo: Mirco -> Micro

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

typo: trunced -> truncated

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

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:05:35Z

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

rui-mo

Thanks! Looks good overall.

Extend funciton fromDateTimeUnitString

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

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.

rui-mo

Thanks. Just several nits.

rui-mo · 2025-03-13T16:26:24Z

nit: perhaps add this case in the test.

rui-mo · 2025-03-13T16:29:51Z

@Yuhta @pedroerp This PR introduces some refactor in the 'functions/lib' and 'functions/presto', would you like to take a look? If no further comment, we can mark it as ready-to-merge. Thanks!

rui-mo

Can you rebase this PR? Thanks.

rui-mo

Can you run the fuzzer test for 2 minutes to make sure it works? You could use --only to specify this function.

Command example: https://github.com/facebookincubator/velox/blob/main/.github/workflows/scheduled.yml#L587-L596

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 · 2025-03-31T12:17:20Z

Can you run the fuzzer test for 2 minutes to make sure it works? You could use --only to specify this function.

Command example: https://github.com/facebookincubator/velox/blob/main/.github/workflows/scheduled.yml#L587-L596

facebook-github-bot · 2025-03-31T17:55:17Z

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

facebook-github-bot · 2025-04-02T20:24:53Z

@bikramSingh91 merged this pull request in f14efd0.

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

Comment thread velox/functions/lib/tests/TimeUtilsTest.cpp Outdated

Copy link
Copy Markdown

Contributor

jinchengchenghh Nov 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test invalid fmt by VELOX_ASSERT_THROW

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

Closed

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

Comment thread velox/functions/lib/TimeUtils.h Outdated

Comment thread velox/functions/lib/TimeUtils.h Outdated

Comment thread velox/functions/sparksql/DateTimeFunctions.h Outdated

Comment thread velox/functions/sparksql/DateTimeFunctions.h Outdated

jinchengchenghh reviewed Feb 26, 2025

View reviewed changes

Comment thread velox/functions/lib/TimeUtils.h Outdated

rui-mo approved these changes Mar 3, 2025

View reviewed changes

Comment thread velox/functions/lib/TimeUtils.h Outdated

Comment thread velox/functions/sparksql/DateTimeFunctions.h Outdated

Comment thread velox/functions/sparksql/DateTimeFunctions.h Outdated

Comment thread velox/docs/functions/spark/datetime.rst Outdated

rui-mo approved these changes Mar 10, 2025

View reviewed changes

rui-mo approved these changes Mar 13, 2025

View reviewed changes

Comment thread velox/docs/functions/spark/datetime.rst Outdated

Copy link
Copy Markdown

Contributor

rui-mo Mar 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: perhaps add this case in the test.

rui-mo reviewed Mar 31, 2025

View reviewed changes

zml1206 added 7 commits March 31, 2025 16:25

add date_trunc spark function

3316cdb

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

update

ec0520a

fix style

7cec3d1

fix

f17ccba

update

d84dd3d

address comments

d9fe097

add test

5fb41e8

zml1206 force-pushed the add_date_trunc branch from 7ab6e05 to 5fb41e8 Compare March 31, 2025 08:25

rui-mo 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 Mar 31, 2025

zml1206 mentioned this pull request Apr 1, 2025

[GLUTEN-7589][VL] Support date_trunc function apache/gluten#7611

Merged

fix

ca45de4

facebook-github-bot closed this in f14efd0 Apr 2, 2025

facebook-github-bot added the Merged label Apr 2, 2025

rui-mo mentioned this pull request Feb 11, 2026

[VL] Support timestamp on Velox backend apache/gluten#1433

Open

34 tasks

Conversation

zml1206 commented Oct 24, 2024 • edited by rui-mo Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

netlify Bot commented Oct 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for meta-velox canceled.

Uh oh!

rui-mo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rui-mo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jinchengchenghh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rui-mo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zml1206 commented Oct 24, 2024 •

edited by rui-mo

Loading

netlify Bot commented Oct 24, 2024 •

edited

Loading