yashmayya commented on code in PR #16885:
URL: https://github.com/apache/pinot/pull/16885#discussion_r2376739532
##########
pinot-spi/src/main/java/org/apache/pinot/spi/utils/ArrayCopyUtils.java:
##########
@@ -267,8 +269,22 @@ public static void copyToBoolean(String[] src, int[] dest,
int length) {
}
public static void copyToTimestamp(String[] src, long[] dest, int length) {
+ copyToTimestamp(src, dest, length, null);
+ }
+
+ public static void copyToTimestamp(String[] src, long[] dest, int length,
RoaringBitmap nullBitmap) {
+ // Skip actual null values because the default null string value won't be
parseable as a valid timestamp.
+ if (nullBitmap != null) {
+ // Prefer this approach to calling RoaringBitmap::contains in a loop for
better performance.
+ nullBitmap.forEach((IntConsumer) nullIdx -> dest[nullIdx] = -1);
+ }
for (int i = 0; i < length; i++) {
- dest[i] = TimestampUtils.toMillisSinceEpoch(src[i]);
+ if (dest[i] != -1) {
+ dest[i] = TimestampUtils.toMillisSinceEpoch(src[i]);
+ }
+ }
+ if (nullBitmap != null) {
+ nullBitmap.forEach((IntConsumer) nullIdx -> dest[nullIdx] = 0);
Review Comment:
This may be a valid point, I see [this
value](https://github.com/apache/pinot/blob/4ba63e2b8d1877cd9476ac68aebba2bcbefbbbe9/pinot-spi/src/main/java/org/apache/pinot/spi/utils/TimeUtils.java#L37)
being used in some places, but I'm not sure what's the significance of using 1
year after the Unix epoch as the min timestamp.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
