xiangfu0 commented on code in PR #20246:
URL: https://github.com/apache/pinot/pull/20246#discussion_r4556398444
##########
pinot-common/src/main/java/org/apache/pinot/common/function/scalar/DateTimeFunctions.java:
##########
@@ -1100,18 +1100,20 @@ public static Timestamp dateBin(String binWidthStr,
Timestamp sourceTimestamp, T
return new Timestamp(binnedMillis);
}
- /// Utility method to aligns a given timestamp in epoch Millis to the
nearest bin defined by the specified
- /// duration string, starting from an origin timestamp in epoch Millis.
+ /// Utility method to align a given timestamp in epoch millis to the bin
boundary at or before it, where bins are
+ /// defined by the specified duration string and anchored at an origin
timestamp in epoch millis.
///
/// @param binWidthStr The width of each bin as an ISO-8601 duration string
(e.g., "PT15M" for 15 minutes).
/// @param sourceMillisEpoch The source time in epoch millis to be aligned.
/// @param originMillisEpoch The origin time in epoch millis from which
binning starts.
- /// @return A java.sql.Timestamp aligned to the nearest bin.
+ /// @return The bin boundary at or before the source, in epoch millis.
public static long dateBin(String binWidthStr, long sourceMillisEpoch, long
originMillisEpoch) {
long binWidthMillis = TimeUtils.convertPeriodToMillis(binWidthStr);
Review Comment:
Minor, not blocking: the period formatter accepts signed values, so
`convertPeriodToMillis("-15m")` returns `-900000`. With `floorDiv` and a
negative width, a source 100s *after* the origin now bins to `origin + 15m`,
which is after the source. The old code returned `origin`. So "unchanged for
non-negative offsets" only holds for positive widths.
Consider rejecting non-positive widths here, e.g.
`Preconditions.checkArgument(binWidthMillis > 0, ...)`. That matches PostgreSQL
("stride must be greater than zero") and gives `"0s"` a clear error instead of
`/ by zero`.
AI-generated review via IntentLab ยท Model: claude-opus-5-5.
--
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]
