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/05/31 11:15:39 UTC
[GitHub] [arrow] jvictorhuguenin opened a new pull request #10425: ARROW-12910: [Gandiva][C++]Add support for ADD and SUBTRACT functions receiving time intervals
jvictorhuguenin opened a new pull request #10425:
URL: https://github.com/apache/arrow/pull/10425
…r month intervals
--
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
[GitHub] [arrow] jvictorhuguenin commented on a change in pull request #10425: ARROW-12910: [Gandiva][C++]Add support for ADD and SUBTRACT functions receiving time intervals
Posted by GitBox <gi...@apache.org>.
jvictorhuguenin commented on a change in pull request #10425:
URL: https://github.com/apache/arrow/pull/10425#discussion_r782658799
##########
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 a time interval to timestamp
+ADD_DAY_TIME_INTERVAL_TO_DATE_TYPES(date64, add, MILLIS_IN_DAY)
Review comment:
fixed
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] jvictorhuguenin commented on a change in pull request #10425: ARROW-12910: [Gandiva][C++]Add support for ADD and SUBTRACT functions receiving time intervals
Posted by GitBox <gi...@apache.org>.
jvictorhuguenin commented on a change in pull request #10425:
URL: https://github.com/apache/arrow/pull/10425#discussion_r788842796
##########
File path: cpp/src/gandiva/tests/projector_test.cc
##########
@@ -1606,4 +1606,40 @@ TEST_F(TestProjector, TestCastNullableIntYearInterval) {
EXPECT_ARROW_ARRAY_EQUALS(out_int64, outputs.at(1));
}
+TEST_F(TestProjector, TestAddTimeIntervalsDateTypes) {
+ auto field_year_interval = field("f0", arrow::month_interval());
+ auto field_time = field("f1", arrow::date64());
+ auto schema = arrow::schema({field_time, field_year_interval});
+
+ // output fields
+ auto field_result = field("r0", arrow::date64());
+
+ // Build expression
+ auto expr = TreeExprBuilder::MakeExpression("add", {field_time, field_year_interval},
Review comment:
done
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] jvictorhuguenin commented on a change in pull request #10425: ARROW-12910: [Gandiva][C++]Add support for ADD and SUBTRACT functions receiving time intervals
Posted by GitBox <gi...@apache.org>.
jvictorhuguenin commented on a change in pull request #10425:
URL: https://github.com/apache/arrow/pull/10425#discussion_r782420476
##########
File path: cpp/src/gandiva/precompiled/types.h
##########
@@ -571,4 +572,37 @@ gdv_month_interval castNULLABLEINTERVALYEAR_int32(int64_t context, gdv_int32 in)
gdv_month_interval castNULLABLEINTERVALYEAR_int64(int64_t context, gdv_int64 in);
+gdv_int64 extractMillis_daytimeinterval(gdv_day_time_interval in);
Review comment:
Because they need to be in types.h so I can use them intimestamp_arithmetic.cc, otherwise It would result in a link error
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] jvictorhuguenin commented on a change in pull request #10425: ARROW-12910: [Gandiva][C++]Add support for ADD and SUBTRACT functions receiving time intervals
Posted by GitBox <gi...@apache.org>.
jvictorhuguenin commented on a change in pull request #10425:
URL: https://github.com/apache/arrow/pull/10425#discussion_r685614811
##########
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:
there's already a test case for the add function to check if it's considering then, I modified the test that should test it on subtract to be more effective.
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] jvictorhuguenin commented on a change in pull request #10425: ARROW-12910: [Gandiva][C++]Add support for ADD and SUBTRACT functions receiving time intervals
Posted by GitBox <gi...@apache.org>.
jvictorhuguenin commented on a change in pull request #10425:
URL: https://github.com/apache/arrow/pull/10425#discussion_r685610874
##########
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:
you are right, I'm sorry
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] jvictorhuguenin commented on a change in pull request #10425: ARROW-12910: [Gandiva][C++]Add support for ADD and SUBTRACT functions receiving time intervals
Posted by GitBox <gi...@apache.org>.
jvictorhuguenin commented on a change in pull request #10425:
URL: https://github.com/apache/arrow/pull/10425#discussion_r782659070
##########
File path: cpp/src/gandiva/function_registry_timestamp_arithmetic.cc
##########
@@ -71,8 +71,31 @@ std::vector<NativeFunction> GetDateTimeArithmeticFunctionRegistry() {
DATE_ADD_FNS(date_add, {}),
DATE_ADD_FNS(add, {}),
- NativeFunction("add", {}, DataTypeVector{date64(), int64()}, timestamp(),
+ NativeFunction("add", {}, DataTypeVector{date64(), int64()}, date64(),
kResultNullIfNull, "add_date64_int64"),
+ NativeFunction("add", {}, DataTypeVector{date64(), day_time_interval()}, date64(),
Review comment:
done
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] jvictorhuguenin commented on pull request #10425: ARROW-12910: [Gandiva][C++]Add support for ADD and SUBTRACT functions receiving time intervals
Posted by GitBox <gi...@apache.org>.
jvictorhuguenin commented on pull request #10425:
URL: https://github.com/apache/arrow/pull/10425#issuecomment-895676708
@anthonylouisbsb, applied changes
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] anthonylouisbsb commented on pull request #10425: ARROW-12910: [Gandiva][C++]Add support for ADD and SUBTRACT functions receiving time intervals
Posted by GitBox <gi...@apache.org>.
anthonylouisbsb commented on pull request #10425:
URL: https://github.com/apache/arrow/pull/10425#issuecomment-867247533
One point that I missed is that you added some unit tests, but it would be good to add some `integrated` tests, just to check if the function registry is working correctly
--
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
[GitHub] [arrow] github-actions[bot] commented on pull request #10425: ARROW-12910: [Gandiva][C++]Add support for ADD and SUBTRACT functions receiving time intervals
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #10425:
URL: https://github.com/apache/arrow/pull/10425#issuecomment-851422579
https://issues.apache.org/jira/browse/ARROW-12910
--
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
[GitHub] [arrow] jvictorhuguenin commented on a change in pull request #10425: ARROW-12910: [Gandiva][C++]Add support for ADD and SUBTRACT functions receiving time intervals
Posted by GitBox <gi...@apache.org>.
jvictorhuguenin commented on a change in pull request #10425:
URL: https://github.com/apache/arrow/pull/10425#discussion_r782659114
##########
File path: cpp/src/gandiva/function_registry_timestamp_arithmetic.cc
##########
@@ -71,8 +71,31 @@ std::vector<NativeFunction> GetDateTimeArithmeticFunctionRegistry() {
DATE_ADD_FNS(date_add, {}),
DATE_ADD_FNS(add, {}),
- NativeFunction("add", {}, DataTypeVector{date64(), int64()}, timestamp(),
+ NativeFunction("add", {}, DataTypeVector{date64(), int64()}, date64(),
kResultNullIfNull, "add_date64_int64"),
+ NativeFunction("add", {}, DataTypeVector{date64(), day_time_interval()}, date64(),
+ kResultNullIfNull, "add_day_time_interval_date64"),
Review comment:
done
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] jvictorhuguenin commented on a change in pull request #10425: ARROW-12910: [Gandiva][C++]Add support for ADD and SUBTRACT functions receiving time intervals
Posted by GitBox <gi...@apache.org>.
jvictorhuguenin commented on a change in pull request #10425:
URL: https://github.com/apache/arrow/pull/10425#discussion_r782659277
##########
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_DATE_TYPES(TYPE, NAME, TO_MILLIS) \
+ FORCE_INLINE \
+ gdv_##TYPE 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_##TYPE>(millis) + \
Review comment:
done
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[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
Posted by GitBox <gi...@apache.org>.
anthonylouisbsb commented on a change in pull request #10425:
URL: https://github.com/apache/arrow/pull/10425#discussion_r788890693
##########
File path: cpp/src/gandiva/precompiled/time_test.cc
##########
@@ -397,6 +424,36 @@ 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_timestamp_month_interval(StringToTimestamp("2000-02-27 00:00:00"), 4),
Review comment:
Ditto
##########
File path: cpp/src/gandiva/precompiled/time_test.cc
##########
@@ -382,6 +386,29 @@ 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_timestamp_month_interval(StringToTimestamp("2000-02-27 00:00:00"), 4),
Review comment:
Another test case, for leap years, if the date is `2016-01-29` and you add one month, check if the result will be `2016-01-28`
##########
File path: cpp/src/gandiva/precompiled/time_test.cc
##########
@@ -382,6 +386,29 @@ 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_timestamp_month_interval(StringToTimestamp("2000-02-27 00:00:00"), 4),
Review comment:
What happens if the user passes a negative number to this function? It works or raise an error?
##########
File path: cpp/src/gandiva/precompiled/time_test.cc
##########
@@ -382,6 +386,29 @@ 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_timestamp_month_interval(StringToTimestamp("2000-02-27 00:00:00"), 4),
Review comment:
Another test case is using a huge number to add months, like the limit of the integer
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] jvictorhuguenin commented on a change in pull request #10425: ARROW-12910: [Gandiva][C++]Add support for ADD and SUBTRACT functions receiving time intervals
Posted by GitBox <gi...@apache.org>.
jvictorhuguenin commented on a change in pull request #10425:
URL: https://github.com/apache/arrow/pull/10425#discussion_r783343739
##########
File path: cpp/src/gandiva/precompiled/time_test.cc
##########
@@ -345,6 +346,25 @@ 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:
done
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] jvictorhuguenin commented on a change in pull request #10425: ARROW-12910: [Gandiva][C++]Add support for ADD and SUBTRACT functions receiving time intervals
Posted by GitBox <gi...@apache.org>.
jvictorhuguenin commented on a change in pull request #10425:
URL: https://github.com/apache/arrow/pull/10425#discussion_r790089526
##########
File path: cpp/src/gandiva/precompiled/time_test.cc
##########
@@ -382,6 +386,29 @@ 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_timestamp_month_interval(StringToTimestamp("2000-02-27 00:00:00"), 4),
Review comment:
Done, I couldn't use the limit for INT32 on month_interval because if I did it it would overflow the timestamp result, since it's adds INT32_MAX*millis_in_a_month, so the result would be to big to int64 to handle. Also I did not added tests for daytime intervals in the type limit because it returns some timestamps too big to make even sense like 185543538537600000
##########
File path: cpp/src/gandiva/precompiled/time_test.cc
##########
@@ -397,6 +424,36 @@ 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_timestamp_month_interval(StringToTimestamp("2000-02-27 00:00:00"), 4),
Review comment:
done
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] jvictorhuguenin commented on pull request #10425: ARROW-12910: [Gandiva][C++]Add support for ADD and SUBTRACT functions receiving time intervals
Posted by GitBox <gi...@apache.org>.
jvictorhuguenin commented on pull request #10425:
URL: https://github.com/apache/arrow/pull/10425#issuecomment-866786885
@anthonylouisbsb, can you please review this PR?
--
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
[GitHub] [arrow] projjal commented on a change in pull request #10425: ARROW-12910: [Gandiva][C++]Add support for ADD and SUBTRACT functions receiving time intervals
Posted by GitBox <gi...@apache.org>.
projjal commented on a change in pull request #10425:
URL: https://github.com/apache/arrow/pull/10425#discussion_r761109054
##########
File path: cpp/src/gandiva/precompiled/types.h
##########
@@ -571,4 +572,37 @@ gdv_month_interval castNULLABLEINTERVALYEAR_int32(int64_t context, gdv_int32 in)
gdv_month_interval castNULLABLEINTERVALYEAR_int64(int64_t context, gdv_int64 in);
+gdv_int64 extractMillis_daytimeinterval(gdv_day_time_interval in);
Review comment:
why extract ones are added?
##########
File path: cpp/src/gandiva/function_registry_timestamp_arithmetic.cc
##########
@@ -71,8 +71,31 @@ std::vector<NativeFunction> GetDateTimeArithmeticFunctionRegistry() {
DATE_ADD_FNS(date_add, {}),
DATE_ADD_FNS(add, {}),
- NativeFunction("add", {}, DataTypeVector{date64(), int64()}, timestamp(),
+ NativeFunction("add", {}, DataTypeVector{date64(), int64()}, date64(),
kResultNullIfNull, "add_date64_int64"),
+ NativeFunction("add", {}, DataTypeVector{date64(), day_time_interval()}, date64(),
Review comment:
Better create a macro for these
##########
File path: cpp/src/gandiva/function_registry_timestamp_arithmetic.cc
##########
@@ -71,8 +71,31 @@ std::vector<NativeFunction> GetDateTimeArithmeticFunctionRegistry() {
DATE_ADD_FNS(date_add, {}),
DATE_ADD_FNS(add, {}),
- NativeFunction("add", {}, DataTypeVector{date64(), int64()}, timestamp(),
+ NativeFunction("add", {}, DataTypeVector{date64(), int64()}, date64(),
kResultNullIfNull, "add_date64_int64"),
+ NativeFunction("add", {}, DataTypeVector{date64(), day_time_interval()}, date64(),
+ kResultNullIfNull, "add_day_time_interval_date64"),
Review comment:
change the name order for clarity
##########
File path: cpp/src/gandiva/tests/projector_test.cc
##########
@@ -1606,4 +1606,40 @@ TEST_F(TestProjector, TestCastNullableIntYearInterval) {
EXPECT_ARROW_ARRAY_EQUALS(out_int64, outputs.at(1));
}
+TEST_F(TestProjector, TestAddTimeIntervalsDateTypes) {
+ auto field_year_interval = field("f0", arrow::month_interval());
+ auto field_time = field("f1", arrow::date64());
+ auto schema = arrow::schema({field_time, field_year_interval});
+
+ // output fields
+ auto field_result = field("r0", arrow::date64());
+
+ // Build expression
+ auto expr = TreeExprBuilder::MakeExpression("add", {field_time, field_year_interval},
Review comment:
can you add one for each of the functions added.
##########
File path: cpp/src/gandiva/precompiled/time_test.cc
##########
@@ -345,6 +346,25 @@ 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:
Add Unit test for all the function variants
##########
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 a time interval to timestamp
+ADD_DAY_TIME_INTERVAL_TO_DATE_TYPES(date64, add, MILLIS_IN_DAY)
Review comment:
2nd, 3rd argument not required.
Also you can further create a single macro for all add/sub day_time_interval/year_month_interval
##########
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_DATE_TYPES(TYPE, NAME, TO_MILLIS) \
+ FORCE_INLINE \
+ gdv_##TYPE 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_##TYPE>(millis) + \
Review comment:
static_cast here looks redundant
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[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
Posted by GitBox <gi...@apache.org>.
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