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/12/02 18:17:20 UTC
[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
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