Copilot commented on code in PR #16885:
URL: https://github.com/apache/pinot/pull/16885#discussion_r2376731727
##########
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:
Using -1 as a sentinel value to mark null entries is fragile and could
conflict with valid timestamp values. Consider using a more explicit approach
like a separate boolean array to track which indices to skip, or document why
-1 is safe for timestamps.
```suggestion
for (int i = 0; i < length; i++) {
if (nullBitmap != null && nullBitmap.contains(i)) {
dest[i] = 0;
} else {
dest[i] = TimestampUtils.toMillisSinceEpoch(src[i]);
}
}
```
##########
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) {
Review Comment:
The condition `dest[i] != -1` assumes the destination array is initialized
to -1 for null values, but this check happens after the null bitmap processing.
This logic is confusing and could break if the destination array has other
initialization values. Consider using the null bitmap directly in the loop
condition instead.
```suggestion
if (nullBitmap == null || !nullBitmap.contains(i)) {
```
##########
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:
Setting null values to 0 after processing may not be the correct default
null representation for timestamps in Pinot. This hardcoded value should be
documented or made configurable, as it could affect how null timestamps are
interpreted elsewhere in the system.
--
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]
