[To dev/1.3] Harden timestamp arithmetic and boundary handling across IoTDB - #18589
[To dev/1.3] Harden timestamp arithmetic and boundary handling across IoTDB#18589Caideyipi wants to merge 3 commits into
Conversation
JackieTien97
left a comment
There was a problem hiding this comment.
Requesting changes for four reproducible correctness regressions in the boundary handling changes. The inline comments include concrete inputs, observed results, and suggested fixes.
Validation: the clean DataNode reactor build and 104 targeted tests passed (24 in node-commons and 80 in datanode; Surefire runs each set twice). Four additional regression tests fail on f24d8a4 and pass when the four affected classes are replaced with their implementations from base commit 9b49621. These are unit-level reproductions, including SQL parsing and the production UDF/fill/inference methods.
| y1 = (double) currentValue - (double) lastValue; | ||
| y2 = (double) nextValue - (double) currentValue; |
There was a problem hiding this comment.
[P2] Preserve the integer difference before converting INT64 values to double
Casting each operand before subtraction loses small differences between large INT64 values. With timestamps [0, 1, 2, 3, 4], values B + [0, 1, 10, 1, 0] where B = 1L << 60, and number=1, proportion=0.2, both cos and prenextdis select timestamp 1 on this commit instead of the unique central spike at timestamp 2. All five values round to the same double, so the value differences become zero. The previous implementations select timestamp 2 for both modes. Please use an overflow-safe integer difference before converting to double, as timeDifferenceAsDouble already does for timestamps, and apply the same correction to the INT64 prenextdis path.
There was a problem hiding this comment.
Fixed in 7b3c9a2. INT64 deltas now use timeDifferenceAsDouble(currentValue, lastValue) before the floating-point cosine calculation, and the same correction is applied to the INT64 prenextdis path. This preserves small differences around 1L << 60.
| return timeDifferenceAsDouble(time, previousTime) <= timeInterval | ||
| && timeDifferenceAsDouble(previousTime, time) <= timeInterval; |
There was a problem hiding this comment.
[P2] Keep the fill interval comparison exact
Using a double for this integer threshold comparison accepts gaps that exceed the configured interval once the distance is above 2^53. For example, at nanosecond precision, set timeInterval=15552000000000000L (180 days), previousTime=1700000000000000000L, and time=previousTime+timeInterval+1. needFill now returns true, whereas the previous implementation returns false: the 180-day-plus-1-ns gap rounds down to the same double as the threshold. This fills a value outside the requested range. Please compare the integer distance exactly with overflow handling; the unsigned-distance comparison added in DownSamplingTimeUtils is one applicable approach.
There was a problem hiding this comment.
Fixed in 7b3c9a2. FixedIntervalFillFilter now delegates to the exact unsigned-distance comparison used by DownSamplingTimeUtils, so the threshold remains exact even above 2^53 and across long subtraction overflow.
| Math.addExact( | ||
| total, | ||
| DateTimeUtils.convertDurationStrToLong( | ||
| currentTime == -1 ? -1 : Math.addExact(currentTime, total), |
There was a problem hiding this comment.
[P2] Do not reject a valid subtraction because an unused addition overflows
This method is also called when parsing date subtraction, but every component of a duration now checks currentTime + total. At nanosecond precision with UTC, select s1 from root.sg.d1 where time > 2262-04-11T23:47:16.854775806 - 1s1ns throws Time duration is out of range. when parsing the second component. The final timestamp is the valid value 9223372035854775805, and the equivalent expression using - 1000000001ns succeeds. The previous implementation accepts both. For fixed-duration units this reference timestamp is not needed; please avoid the unconditional addition and let the caller check the final addition/subtraction, while handling any calendar-dependent reference calculation according to the actual operation.
There was a problem hiding this comment.
Fixed in 7b3c9a2. Duration parsing now passes a reference timestamp only for calendar-dependent month units; fixed-duration components are evaluated without the intermediate currentTime + total calculation. The final date-expression addition/subtraction remains checked by the caller, so a valid subtraction is no longer rejected by an unused intermediate overflow.
|
|
||
| static long calculateGeneratedTime(long maxTimestamp, long interval, long currentRowIndex) { | ||
| try { | ||
| return Math.addExact(maxTimestamp, Math.multiplyExact(interval, currentRowIndex)); |
There was a problem hiding this comment.
[P2] Check the complete generated timestamp rather than the intermediate product
A negative maxTimestamp can bring an intermediate product larger than Long.MAX_VALUE back into the valid timestamp range. For two input timestamps -6000000000000000000L and -4000000000000000000L, the interval calculation yields 1000000000000000000L. If the model returns 11 rows, index 10 should receive timestamp 6000000000000000000L, and every generated timestamp is representable. The new multiplyExact instead throws before adding the negative maximum timestamp, causing fillTimeColumn to fail with Generated time column is out of range.; the previous implementation completes this case. Please evaluate the entire multiply-and-add expression with BigInteger and range-check its final result.
There was a problem hiding this comment.
Fixed in 7b3c9a2. Generated timestamps are now evaluated as maxTimestamp + interval * rowIndex in BigInteger, with the completed value range-checked before conversion. This allows a mathematically valid final timestamp even when the intermediate product alone exceeds long range, while still rejecting an out-of-range final result.
Backport of 2c0c307 to dev/1.3. Adapted for APIs and modules available on 1.3 while preserving the timestamp arithmetic, TTL, partition-boundary, query-limit, memtable, compaction, Pipe, and downsampling fixes. Validation: node-commons boundary/date-time tests passed (24 tests); DataNode targeted tests passed (60 tests per configured Surefire execution); DataNode reactor compile passed.