feat: Add Spark make_dt_interval function by ArnavBalyan · Pull Request #12763 · facebookincubator/velox

ArnavBalyan · 2025-03-23T02:49:47Z

Add day_time_interval support for Spark.
Supports variety of signatures which resolve to the internal representation of milliseconds.
Also carries minor change to make it arrow compatible.
https://github.com/apache/spark/blob/branch-3.5/sql/api/src/main/scala/org/apache/spark/sql/types/DayTimeIntervalType.scala#L70

netlify · 2025-03-23T02:50:06Z

✅ Deploy Preview for meta-velox canceled.

Name	Link
🔨 Latest commit	`ee426dc`
🔍 Latest deploy log	https://app.netlify.com/sites/meta-velox/deploys/6808c1894b521b0008ce4dfd

rui-mo · 2025-03-25T13:33:57Z

Could you please add the link of corresponding Spark's implementation to the PR description? And also remember to update the doc.

Yes here it is https://github.com/apache/spark/blob/branch-3.5/sql/api/src/main/scala/org/apache/spark/sql/types/DayTimeIntervalType.scala#L70

I think https://github.com/apache/spark/blob/branch-3.5/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/intervalExpressions.scala#L391 is the Spark's implementation for this function.

ArnavBalyan · 2025-03-27T13:09:36Z

Addressed the comments

rui-mo · 2025-03-28T15:20:21Z

It looks like the fourth parameter is of short decimal type, so the input type should be int64_t, right?

I think with int64 we lose the decimal information, spark expects double so need to keep the registration as shortdecimal and double here to preserve the timestamp values

rui-mo

Please also remember to update the document. Thanks.

ArnavBalyan · 2025-04-13T14:45:03Z

Please also remember to update the document. Thanks.

updated

rui-mo · 2025-04-16T16:43:31Z

Would you add a test for the bridge change?

rui-mo · 2025-04-16T17:04:44Z

Overflow can already happen during one of these operations before the check. Perhaps use safe arithmetic functions including __builtin_mul_overflow and __builtin_add_overflow to detect overflow during calculations.

rui-mo · 2025-04-16T17:10:44Z

Consider adding a separate file for this implementation as the DateTimeFunctions.h is large.

ArnavBalyan · 2025-04-24T10:32:36Z

cc @rui-mo could you please run this test also thanks

rui-mo

Added some comments. Thanks.

rui-mo · 2025-04-29T16:29:29Z

        SELECT make_ym_interval(2); -- 2-0
        SELECT make_ym_interval(); -- 0-0

+.. spark:function:: make_dt_interval([days[, hours[, minutes[, seconds]]]]) -> interval day to second


Please follow the alphabetical order to add documents, thanks.

rui-mo · 2025-04-29T16:32:00Z

+      IntervalDayTime,
+      int32_t,
+      int32_t,
+      int32_t>({prefix + "make_dt_interval"});


Spark would provide default values for the optional variables, so we might only need to register the four-arg one.

rui-mo · 2025-04-29T16:32:51Z

        SELECT make_ym_interval(2); -- 2-0
        SELECT make_ym_interval(); -- 0-0

+.. spark:function:: make_dt_interval([days[, hours[, minutes[, seconds]]]]) -> interval day to second


ditto for the document of optional variables.

rui-mo · 2025-04-29T16:34:59Z

+
+    Make day-time interval from ``days``, ``hours``, ``minutes`` and ``seconds`` fields.
+    All parameters can be zero, positive or negative.
+    Throws an error when inputs lead to int overflow.


I assume not all parameters are integer, perhaps clarify the parameter types and the overflow behavior.

rui-mo · 2025-04-29T16:36:28Z

+
+namespace facebook::velox::functions::sparksql {
+
+inline int64_t safeMul(int64_t a, int64_t b, const char* context) {


There is the checkedMultiply function, can we reuse it?

rui-mo · 2025-04-29T16:37:14Z

+  return result;
+}
+
+inline int64_t safeAdd(int64_t a, int64_t b, const char* context) {


Can we reuse checkedPlus?

rui-mo · 2025-04-29T16:38:33Z

@@ -0,0 +1,83 @@
+/*


Perhaps rename this header as MakeDTInterval. It is recommended to add a separate file for each function.

rui-mo · 2025-04-29T16:38:59Z

+}
+
+template <typename T>
+struct MakeDTIntervalFunction {


nit: perhaps add some comments for this struct.

rui-mo · 2025-04-29T16:44:25Z

+      int32_t,
+      int32_t,
+      int32_t,
+      ShortDecimal<P1, S1>>({prefix + "make_dt_interval"});


The decimal type seems to be fixed in Spark and is decimal(18, 6).

rui-mo · 2025-04-29T16:46:22Z

+      int32_t days = 0,
+      int32_t hours = 0,
+      int32_t minutes = 0,
+      double secondsMicros = 0) {


I assume the fourth argument is the unscaled value of short decimal type with scale 6, and its C++ type is int64_t.

stale · 2025-07-28T20:07:37Z

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!

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 Mar 23, 2025

rui-mo reviewed Mar 25, 2025

View reviewed changes

rui-mo changed the title ~~feat: Add support for day time interval for Spark~~ feat: Add Spark make_dt_interval function Mar 25, 2025

rui-mo reviewed Mar 28, 2025

View reviewed changes

ArnavBalyan force-pushed the arnavb/v-mk-dt-support branch from ef89d54 to 072db35 Compare April 13, 2025 14:43

rui-mo reviewed Apr 16, 2025

View reviewed changes

update

e3b4d55

ArnavBalyan force-pushed the arnavb/v-mk-dt-support branch from 072db35 to e3b4d55 Compare April 17, 2025 11:07

Merge branch 'main' into arnavb/v-mk-dt-support

da3f65c

ArnavBalyan force-pushed the arnavb/v-mk-dt-support branch from cd2fe35 to da3f65c Compare April 17, 2025 11:30

format

ee426dc

rui-mo reviewed Apr 29, 2025

View reviewed changes

stale Bot added the stale label Jul 28, 2025

stale Bot closed this Aug 11, 2025

FelixYBW mentioned this pull request Feb 7, 2026

[VL] useful Velox PRs not merged into upstream apache/gluten#11585

Open


		namespace facebook::velox::functions::sparksql {

		inline int64_t safeMul(int64_t a, int64_t b, const char* context) {

Conversation

ArnavBalyan commented Mar 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

netlify Bot commented Mar 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for meta-velox canceled.

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!

ArnavBalyan commented Mar 27, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rui-mo left a comment

Choose a reason for hiding this comment

Uh oh!

ArnavBalyan commented Apr 13, 2025

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!

ArnavBalyan commented Apr 24, 2025

Uh oh!

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

stale Bot commented Jul 28, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ArnavBalyan commented Mar 23, 2025 •

edited

Loading

netlify Bot commented Mar 23, 2025 •

edited

Loading