[GitHub] [incubator-pinot] Jackie-Jiang commented on a change in pull request #7116: Add datetime function with 2 arguments

[GitBox <gi...@apache.org>]

Jackie-Jiang commented on a change in pull request #7116:
URL: https://github.com/apache/incubator-pinot/pull/7116#discussion_r662670808



##########
File path: pinot-common/src/main/java/org/apache/pinot/common/function/scalar/DateTimeFunctions.java
##########
@@ -593,6 +593,18 @@ public static int millisecond(long millis, String timezoneId) {
     return new DateTime(millis, DateTimeZone.forID(timezoneId)).getMillisOfSecond();
   }
 
+  /**
+   * The sql compatible date_trunc function for epoch time
+   * @param unit truncate to unit (millisecond, second, minute, hour, day, week, month, quarter, year)
+   * @param timeValue value to truncate
+   * @return truncated timeValue in TimeUnit.MILLISECONDS
+   */
+  @ScalarFunction
+  public static Timestamp dateTrunc(String unit, long timeValue){

Review comment:
       Let's return `long` here for consistency. We can change all 4 functions to return `Timestamp` if needed.

##########
File path: pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/DateTruncTransformFunction.java
##########
@@ -75,6 +76,7 @@
  *   </li>
  * </ul>
  */
+@Deprecated

Review comment:
       We should not deprecate this, but adding the support for 2 arguments as well.
   Implementing a `TransformFunction` has much better performance than modeling it as scalar function.




-- 
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: commits-unsubscribe@pinot.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org