gortiz commented on code in PR #14298:
URL: https://github.com/apache/pinot/pull/14298#discussion_r1816504749
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/DateTimeConversionTransformFunction.java:
##########
@@ -93,10 +113,14 @@ public String getName() {
@Override
public void init(List<TransformFunction> arguments, Map<String,
ColumnContext> columnContextMap) {
super.init(arguments, columnContextMap);
- // Check that there are exactly 4 arguments
- if (arguments.size() != 4) {
- throw new IllegalArgumentException("Exactly 4 arguments are required for
DATE_TIME_CONVERT transform function");
+ String bucketTimeZone = null;
Review Comment:
nit: let's try to use immutable variables. Can you move this null
initialization to its own `else` branch in the if below?
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/transform/transformer/datetime/EpochToEpochTransformer.java:
##########
@@ -29,14 +30,20 @@
public class EpochToEpochTransformer extends BaseDateTimeTransformer<long[],
long[]> {
public EpochToEpochTransformer(DateTimeFormatSpec inputFormat,
DateTimeFormatSpec outputFormat,
- DateTimeGranularitySpec outputGranularity) {
- super(inputFormat, outputFormat, outputGranularity);
+ DateTimeGranularitySpec outputGranularity, DateTimeZone bucketingTz) {
Review Comment:
Remember to add nullability annotations in all these constructors
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/transform/transformer/datetime/DateTimeTransformerFactory.java:
##########
@@ -29,23 +30,39 @@ private DateTimeTransformerFactory() {
public static BaseDateTimeTransformer getDateTimeTransformer(String
inputFormatStr, String outputFormatStr,
String outputGranularityStr) {
+ return getDateTimeTransformer(inputFormatStr, outputFormatStr,
outputGranularityStr, null);
+ }
+
+ public static BaseDateTimeTransformer getDateTimeTransformer(String
inputFormatStr, String outputFormatStr,
+ String outputGranularityStr, String bucketTimeZoneStr) {
Review Comment:
Can you add `@Nullable` here please?
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/transform/transformer/datetime/DateTimeTransformerFactory.java:
##########
@@ -29,23 +30,39 @@ private DateTimeTransformerFactory() {
public static BaseDateTimeTransformer getDateTimeTransformer(String
inputFormatStr, String outputFormatStr,
String outputGranularityStr) {
+ return getDateTimeTransformer(inputFormatStr, outputFormatStr,
outputGranularityStr, null);
+ }
+
+ public static BaseDateTimeTransformer getDateTimeTransformer(String
inputFormatStr, String outputFormatStr,
+ String outputGranularityStr, String bucketTimeZoneStr) {
+
DateTimeFormatSpec inputFormat = new DateTimeFormatSpec(inputFormatStr);
DateTimeFormatSpec outputFormat = new DateTimeFormatSpec(outputFormatStr);
DateTimeGranularitySpec outputGranularity = new
DateTimeGranularitySpec(outputGranularityStr);
+ DateTimeZone bucketingTz = null;
+ if (bucketTimeZoneStr != null) {
+ try {
+ // we're not using TimeZone.getTimeZone() because it's globally
synchronized and returns default TZ when str
Review Comment:
Couldn't we use TimeZone.getTimeZone(ZoneId)?
--
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-unsubscr...@pinot.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
