projjal commented on a change in pull request #9873:
URL: https://github.com/apache/arrow/pull/9873#discussion_r609459002



##########
File path: cpp/src/gandiva/function_registry_common.h
##########
@@ -174,6 +174,15 @@ typedef std::unordered_map<const FunctionSignature*, const 
NativeFunction*, KeyH
   NativeFunction(#NAME, std::vector<std::string> ALIASES, 
DataTypeVector{TYPE()}, \
                  date64(), kResultNullIfNull, 
ARROW_STRINGIFY(NAME##_from_##TYPE))
 
+// To time (used with data/time types) that :
+// - NULL handling is of type NULL_IF_NULL
+//
+// The pre-compiled fn name includes the base name & input type name. eg:
+// - to_time_date64
+#define TO_TIME_SAFE_NULL_IF_NULL(NAME, ALIASES, TYPE)                         
   \

Review comment:
       better to move this macro to registry_datetime.cc

##########
File path: cpp/src/gandiva/precompiled/time.cc
##########
@@ -817,4 +817,19 @@ gdv_int64 castBIGINT_daytimeinterval(gdv_day_time_interval 
in) {
          extractDay_daytimeinterval(in) * MILLIS_IN_DAY;
 }
 
+// Convert milliseconds to date, considering as input the quantity of 
milliseconds
+// after midnight

Review comment:
       quantity of millis since midnight is defined to be time. I think you are 
supposed to convert millis since epoch to time.
   You can also add another function to_date which would return the 
corresponding date

##########
File path: cpp/src/gandiva/function_registry_common.h
##########
@@ -174,6 +174,15 @@ typedef std::unordered_map<const FunctionSignature*, const 
NativeFunction*, KeyH
   NativeFunction(#NAME, std::vector<std::string> ALIASES, 
DataTypeVector{TYPE()}, \
                  date64(), kResultNullIfNull, 
ARROW_STRINGIFY(NAME##_from_##TYPE))
 
+// To time (used with data/time types) that :
+// - NULL handling is of type NULL_IF_NULL
+//
+// The pre-compiled fn name includes the base name & input type name. eg:
+// - to_time_date64
+#define TO_TIME_SAFE_NULL_IF_NULL(NAME, ALIASES, TYPE)                         
   \
+  NativeFunction(#NAME, std::vector<std::string> ALIASES, 
DataTypeVector{TYPE()}, \
+                 date64(), kResultNullIfNull, ARROW_STRINGIFY(NAME##_##TYPE))

Review comment:
       output should be time instead of date

##########
File path: cpp/src/gandiva/precompiled/time.cc
##########
@@ -817,4 +817,19 @@ gdv_int64 castBIGINT_daytimeinterval(gdv_day_time_interval 
in) {
          extractDay_daytimeinterval(in) * MILLIS_IN_DAY;
 }
 
+// Convert milliseconds to date, considering as input the quantity of 
milliseconds
+// after midnight
+#define TO_TIME(TYPE)                                                          
         \
+  FORCE_INLINE                                                                 
         \
+  gdv_date64 to_time##_##TYPE(gdv_##TYPE millis) {                             
         \
+    EpochTimePoint actual_tp(EpochTimePoint::NowMillisEpoch());                
         \

Review comment:
       why is current time need to be used? The argument is already provided

##########
File path: cpp/src/gandiva/precompiled/time.cc
##########
@@ -817,4 +817,19 @@ gdv_int64 castBIGINT_daytimeinterval(gdv_day_time_interval 
in) {
          extractDay_daytimeinterval(in) * MILLIS_IN_DAY;
 }
 
+// Convert milliseconds to date, considering as input the quantity of 
milliseconds
+// after midnight
+#define TO_TIME(TYPE)                                                          
         \
+  FORCE_INLINE                                                                 
         \
+  gdv_date64 to_time##_##TYPE(gdv_##TYPE millis) {                             
         \

Review comment:
       output is not date but time




-- 
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:
[email protected]


Reply via email to