Hi all, I found a correctness bug in date_bin (and date_bin_gapfill) when the interval contains a month or a year. The bin boundary is shifted later by the whole milliseconds already present inside that second. Hour and day bins are not affected.
issue link: https://github.com/apache/iotdb/issues/19264 Version: master @ 8a2f85adf4a37b4a70a90bf456bd8e98e37eb28d (2.0.11-SNAPSHOT). The same code is on the 2.0.11 release commit 7c0de06. 1. What happens date_bin(1mo, time, 500) should keep the origin 1970-01-01T00:00:00.500Z. It returns 1970-01-01T00:00:01.000Z instead. The next month is 1970-02-01T00:00:01.000Z instead of 1970-02-01T00:00:00.500Z. Each boundary is 500 ms late, so a point near the boundary falls into the wrong month. The same shift happens in any year. An origin of 2024-03-15T00:00:00.500Z moves the March and April bins 500 ms later. With timestamp_precision=us, an origin of 1.5 ms becomes 2.5 ms: the extra amount is exactly the 1 ms that was already counted. date_bin(1mo, time) with the default origin 0 stays correct, because every month boundary is an exact second. 2. Why DateBinFunctionColumnTransformer.convertToTimestamp mixes two ways of splitting a timestamp. toEpochMilli() already includes the whole milliseconds of that second. getNano() is the full nanosecond-of-second (0..999_999_999), not the remainder below one millisecond. The method adds them together, so those milliseconds are added twice: ms: toEpochMilli() + getNano() / 1_000_000 us: toMicros(toEpochMilli()) + getNano() / 1_000 ns: toNanos(toEpochMilli()) + getNano() For 1970-01-01T00:00:00.500Z under ms precision this is 500 + 500 = 1000. When getNano() is 0 the second term is 0, which is why the existing whole-second tests pass. MonthIntervalMSFillFilter does not have this problem. It uses plusMonths(...).toInstant().toEpochMilli() and does not add getNano() again. 3. Proposed fix Change only the private method convertToTimestamp. Encode the Instant from getEpochSecond() and getNano(), which is the split the comment already describes: ms: seconds * 1000 + getNano() / 1_000_000 us: seconds * 1_000_000 + getNano() / 1_000 ns: seconds * 1_000_000_000 + getNano() Instant stores a negative time as a negative second plus a non-negative nano, so this also stays correct before 1970. No caller changes. 4. What this does and does not change The method has three callers, all on the month path: dateBin, dateBinStartEnd, and nextDateBin(int, ZoneId, long). The last one is the month step of date_bin_gapfill. They share one conversion, so the query result and the gap-fill step stay on the same boundary. Unchanged: - 1h, 1d, and other non-month bins. They use integer arithmetic. - Non-month gap fill. - The parser rule that rejects a mixed interval such as 1mo1d. - Insert, TsFile, tree model, and authorization. - date_bin(1mo, time) whose boundary is an exact second. The number stays the same. The only behavior change is a month or year bin whose boundary has a fractional second, for example date_bin(1mo, time, 500). That boundary moves back to the correct time. plusMonths and the DST handling are unchanged. 5. Tests I will add unit tests in DateBinFunctionTest: - ms: origin 500 ms, the same point one month later, and dateBinStartEnd - the same case on 2024-03-15 - an exact-second origin, which must stay unchanged - us and ns: origin 1.5 ms, which must stay 1.5 ms rather than 2.5 ms Existing DateBinFunctionTest cases use whole milliseconds and should still pass. There is a separate failure I am not including in this fix: under us or ns precision, convertToLocalDateTime passes a negative remainder to Instant.ofEpochSecond, so a month date_bin on a pre-1970 timestamp that is not an exact second throws DateTimeException. I can file that separately if you want it tracked on its own. I am happy to submit the patch after comments on this approach. Thanks, DaZuiZui
