You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2021/06/24 00:37:00 UTC
[GitHub] [arrow] anthonylouisbsb commented on a change in pull request #10425: ARROW-12910: [Gandiva][C++]Add support for ADD and SUBTRACT functions receiving time intervals
anthonylouisbsb commented on a change in pull request #10425:
URL: https://github.com/apache/arrow/pull/10425#discussion_r657542090
##########
File path: cpp/src/gandiva/precompiled/time_test.cc
##########
@@ -315,6 +316,18 @@ TEST(TestTime, TimeStampAdd) {
EXPECT_EQ(add_date64_int64(StringToTimestamp("2000-02-27 00:00:00"), 4),
StringToTimestamp("2000-03-02 00:00:00"));
+ EXPECT_EQ(
Review comment:
I think it would be good to add a test using leap year, both for `add` and `subtract` functions, to see if it is working correctly in that case.
##########
File path: cpp/src/gandiva/precompiled/timestamp_arithmetic.cc
##########
@@ -239,4 +290,18 @@ ADD_TIMESTAMP_TO_INT64_FIXED_UNITS(date64, add, MILLIS_IN_DAY)
ADD_TIMESTAMP_TO_INT64_FIXED_UNITS(timestamp, date_add, MILLIS_IN_DAY)
ADD_TIMESTAMP_TO_INT64_FIXED_UNITS(timestamp, add, MILLIS_IN_DAY)
+// add timestamp to day time interval
Review comment:
Is this comment correct? The function is adding a day time interval in timestamp, right?
##########
File path: cpp/src/gandiva/precompiled/time_test.cc
##########
@@ -315,6 +316,18 @@ TEST(TestTime, TimeStampAdd) {
EXPECT_EQ(add_date64_int64(StringToTimestamp("2000-02-27 00:00:00"), 4),
StringToTimestamp("2000-03-02 00:00:00"));
+ EXPECT_EQ(
+ add_year_month_interval_timestamp(StringToTimestamp("2000-02-27 00:00:00"), 4),
+ StringToTimestamp("2000-06-27 00:00:00"));
+
+ EXPECT_EQ(add_day_time_interval_timestamp(StringToTimestamp("2000-02-27 00:00:00"), 4),
Review comment:
I think other good test case to add is when the result timestamp are negative values(timestamps before `1970-01-01`)
##########
File path: cpp/src/gandiva/precompiled/timestamp_arithmetic.cc
##########
@@ -239,4 +290,18 @@ ADD_TIMESTAMP_TO_INT64_FIXED_UNITS(date64, add, MILLIS_IN_DAY)
ADD_TIMESTAMP_TO_INT64_FIXED_UNITS(timestamp, date_add, MILLIS_IN_DAY)
ADD_TIMESTAMP_TO_INT64_FIXED_UNITS(timestamp, add, MILLIS_IN_DAY)
+// add timestamp to day time interval
+ADD_DAY_TIME_INTERVAL_TO_TIMESTAMP(date64, add, MILLIS_IN_DAY)
Review comment:
I think you should replace `TIMESTAMP` in the macro names to `DATE_TYPES`
##########
File path: cpp/src/gandiva/precompiled/timestamp_arithmetic.cc
##########
@@ -172,6 +172,57 @@ TIMESTAMP_DIFF(timestamp)
return millis + TO_MILLIS * static_cast<gdv_##TYPE>(count); \
}
+#define ADD_DAY_TIME_INTERVAL_TO_TIMESTAMP(TYPE, NAME, TO_MILLIS) \
+ FORCE_INLINE \
+ gdv_timestamp NAME##_day_time_interval_##TYPE(gdv_##TYPE millis, \
+ gdv_day_time_interval count) { \
+ gdv_int64 day_interval_days = extractDay_daytimeinterval(count); \
+ gdv_int64 day_interval_millis = extractMillis_daytimeinterval(count); \
+ return static_cast<gdv_timestamp>(millis) + \
+ (day_interval_days * TO_MILLIS + day_interval_millis); \
+ }
+
+#define SUB_DAY_TIME_INTERVAL_TO_TIMESTAMP(TYPE, NAME, TO_MILLIS) \
+ FORCE_INLINE \
+ gdv_timestamp NAME##_day_time_interval_##TYPE(gdv_##TYPE millis, \
+ gdv_day_time_interval count) { \
+ gdv_int64 day_interval_days = extractDay_daytimeinterval(count); \
+ gdv_int64 day_interval_millis = extractMillis_daytimeinterval(count); \
+ return static_cast<gdv_timestamp>(millis) - \
+ (day_interval_days * TO_MILLIS + day_interval_millis); \
+ }
+
+#define ADD_DAY_TIME_INTERVAL_TO_TIME(TYPE) \
+ FORCE_INLINE \
+ gdv_time32 add_day_time_interval_##TYPE(gdv_##TYPE millis, \
+ gdv_day_time_interval count) { \
+ millis += static_cast<gdv_##TYPE>(extractMillis_daytimeinterval(count)); \
+ return millis % MILLIS_IN_DAY; \
+ }
+
+#define SUB_DAY_TIME_INTERVAL_TO_TIME(TYPE) \
+ FORCE_INLINE \
+ gdv_time32 subtract_day_time_interval_##TYPE(gdv_##TYPE millis, \
+ gdv_day_time_interval count) { \
+ millis -= static_cast<gdv_##TYPE>(extractMillis_daytimeinterval(count)); \
+ return millis % MILLIS_IN_DAY; \
+ }
+
+#define ADD_YEAR_MONTH_INTERVAL_TO_TIMESTAMP(TYPE) \
+ FORCE_INLINE \
+ gdv_timestamp add_year_month_interval_##TYPE(gdv_##TYPE millis, \
+ gdv_year_month_interval count) { \
+ EpochTimePoint tp(millis); \
+ return static_cast<gdv_timestamp>(tp.AddMonths(count).MillisSinceEpoch()); \
+ }
+
+#define SUB_YEAR_MONTH_INTERVAL_TO_TIMESTAMP(TYPE) \
+ FORCE_INLINE \
+ gdv_timestamp subtract_year_month_interval_##TYPE(gdv_##TYPE millis, \
+ gdv_year_month_interval count) { \
+ EpochTimePoint tp(millis); \
+ return static_cast<gdv_timestamp>(tp.AddMonths(-1 * count).MillisSinceEpoch()); \
+ }
Review comment:
Ditto
##########
File path: cpp/src/gandiva/precompiled/timestamp_arithmetic.cc
##########
@@ -172,6 +172,57 @@ TIMESTAMP_DIFF(timestamp)
return millis + TO_MILLIS * static_cast<gdv_##TYPE>(count); \
}
+#define ADD_DAY_TIME_INTERVAL_TO_TIMESTAMP(TYPE, NAME, TO_MILLIS) \
+ FORCE_INLINE \
+ gdv_timestamp NAME##_day_time_interval_##TYPE(gdv_##TYPE millis, \
+ gdv_day_time_interval count) { \
+ gdv_int64 day_interval_days = extractDay_daytimeinterval(count); \
+ gdv_int64 day_interval_millis = extractMillis_daytimeinterval(count); \
+ return static_cast<gdv_timestamp>(millis) + \
+ (day_interval_days * TO_MILLIS + day_interval_millis); \
+ }
+
+#define SUB_DAY_TIME_INTERVAL_TO_TIMESTAMP(TYPE, NAME, TO_MILLIS) \
+ FORCE_INLINE \
+ gdv_timestamp NAME##_day_time_interval_##TYPE(gdv_##TYPE millis, \
+ gdv_day_time_interval count) { \
+ gdv_int64 day_interval_days = extractDay_daytimeinterval(count); \
+ gdv_int64 day_interval_millis = extractMillis_daytimeinterval(count); \
+ return static_cast<gdv_timestamp>(millis) - \
+ (day_interval_days * TO_MILLIS + day_interval_millis); \
+ }
Review comment:
These macros are always returning a `gdv_timestamp`, but when the functions are receiving a `gdv_date64` they should return a variable of `gdv_date64` too, right?
##########
File path: cpp/src/gandiva/precompiled/time_test.cc
##########
@@ -330,6 +343,19 @@ TEST(TestTime, TimeStampAdd) {
EXPECT_EQ(date_diff_timestamp_int64(StringToTimestamp("2000-02-29 00:00:00"), 365),
StringToTimestamp("1999-03-01 00:00:00"));
+
+ EXPECT_EQ(
+ subtract_year_month_interval_timestamp(StringToTimestamp("2000-02-27 00:00:00"), 4),
+ StringToTimestamp("1999-10-27 00:00:00"));
Review comment:
What happens if the user subtracts a value from a `time32` and it results in a negative value? I think it is a good test case to add
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org