Support rate family aggregation functions in table model - #18334
Support rate family aggregation functions in table model#18334CoollZzz wants to merge 2 commits into
Conversation
JackieTien97
left a comment
There was a problem hiding this comment.
Thanks for adding the table-model rate-family aggregations. I reviewed the implementation against the requirement and technical design and found blocking correctness and memory-accounting issues in the inline comments.
Please also complete the following coverage and delivery items:
- Run behavioral tests under
ms,us, andnstimestamp_precision, including exact-threshold and one-tick-below cases. - Link the companion user-manual change for the table SQL functions/operators if documentation is maintained in a separate repository.
- Add focused unit coverage for the extrapolation math, intermediate-state codec, planner ordered/naive selection, and
AggregationNodeserialization.
Requesting changes because the threshold comparison can return incorrect query results, and the intermediate-state path can exceed the query memory budget.
| double averageInterval = sampledInterval / (sampleCount - 1); | ||
| double threshold = averageInterval * EXTRAPOLATION_THRESHOLD_FACTOR; | ||
|
|
||
| if (durationToStart >= threshold) { |
There was a problem hiding this comment.
[P1] Compare the extrapolation threshold in integer tick space
The contract requires using half the average interval when gap >= 1.1 * averageInterval, but converting each interval independently to double seconds can flip this boundary. At millisecond precision, for window [0, 31) with samples (11, 100) and (21, 110), the left gap is exactly 1.1 * 10 ms. The expected increase/delta is 25 and rate is about 806.4516; here threshold becomes 0.011000000000000001 while the gap is 0.011, so the branch is skipped and the implementation returns 31 and 1000.
Please compare exact ticks before converting to seconds, for example with BigInteger: gapTicks * (sampleCount - 1) * 10 >= sampledTicks * 11. Add regression cases for both boundaries under ms/us/ns precision, covering equality and one tick below the threshold.
| long headerSize = functionType.isWindowed() ? WINDOWED_HEADER_SIZE : IRATE_HEADER_SIZE; | ||
| long serializedSize = | ||
| Math.addExact(headerSize, Math.multiplyExact((long) samples.size(), SAMPLE_SIZE)); | ||
| ByteBuffer target = ByteBuffer.allocate(Math.toIntExact(serializedSize)); |
There was a problem hiding this comment.
[P1] Reserve or bound the serialized rate state before allocating it
This materializes every sample in the group into one additional header + 16 * n byte array while the original TimeValueBuffer is still live. The allocation is not reserved through the query memory manager, a single Binary is not bounded by maxTsBlockSizeInBytes, and AggregationOperator.calculateMaxReturnSize() still advertises only the fixed max TsBlock size. A valid partial aggregation can therefore fit its registered accumulator reservation and then exceed that budget—potentially with a single state approaching 2 GB—while serializing. Decode also retains the input Binary while creating a temporary buffer and merging it into the destination.
Please either chunk/bound the intermediate state or reserve the serialized output and decode-temporary memory before allocation.
| long currentSize = getEstimatedSize(); | ||
| long delta = currentSize - previousSize; | ||
| if (delta > 0) { | ||
| memoryReservationManager.reserveMemoryCumulatively(delta); |
There was a problem hiding this comment.
[P2] Avoid charging grouped rate state twice
This accumulator reserves its full estimated size through the fragment-level memory manager, but HashAggregationOperator and StreamingHashAggregationOperator reserve the same state again through InMemoryHashAggregationBuilder#getEstimatedSize(), which already includes every GroupedAggregator. Both paths resolve to the same FragmentInstanceContext manager, so allocations in all eight grouped rate-family accumulators are charged roughly twice and valid grouped queries can fail with MemoryNotEnoughException while the actual state still fits.
Please make either the grouped accumulator or the enclosing hash operator the single owner of this reservation.
| "聚合函数 [%s] 要求同一聚合分组内的窗口边界一致:期望 [%d, %d),实际为 [%d, %d)"; | ||
| public static final String | ||
| EXCEPTION_AGGREGATE_FUNCTION_ARG_EXPECTED_TIME_COL_IN_STRICTLY_ASCENDING_ORDER_BUT_GOT_ARG_AFTER_ARG_9289E0F9 = | ||
| "聚合函数 [%s] 要求 time_col 严格升序,但在 %d 之后得到了 %d"; |
There was a problem hiding this comment.
[P3] Preserve the current/previous timestamp order in the Chinese message
The callers format this template with (functionName, currentTime, previousTime), matching the English wording got current after previous. This Chinese wording reads the placeholders in the opposite order: when time moves from 2 back to 1, it reports 在 1 之后得到了 2. Please either rewrite it as 但得到了 %d,前一个值为 %d or use positional placeholders that preserve the supplied argument order.
| DATABASE_NAME); | ||
|
|
||
| // The rows span two flushed files and two physical time partitions. In cluster mode this also | ||
| // exercises partial/intermediate/final aggregation. |
There was a problem hiding this comment.
[P2] Exercise an actual multi-stage intermediate merge here
Both fixture batches use the same scenario=distributed TAG. Data-partition inheritance keeps all time slots for the same SeriesSlot in the same DataRegion, so spanning two files and time partitions does not demonstrate the cluster partial/intermediate/final merge claimed by this comment.
Please use multiple tag/device values placed in different DataRegions while aggregating them into the same SQL group, and assert the placement or physical plan so this test reliably covers merging multiple intermediate states.
| "SELECT window_start,window_end," | ||
| + "rate(value_double,time,window_start,window_end) AS rate_value " | ||
| + "FROM HOP(DATA => (SELECT time,value_double FROM rate_test " | ||
| + "WHERE scenario='timestamp_normal'),TIMECOL => 'time',SLIDE => 5ms,SIZE => 5ms) " |
There was a problem hiding this comment.
[P2] Cover an overlapping HOP window
With SLIDE => 5ms and SIZE => 5ms, no input row belongs to multiple windows, so this exercises the same window shape as TUMBLE rather than the overlapping HOP scenario required by the specification. Please add a case with SLIDE < SIZE and verify the rate-family results for every overlapping window.
70ff39c to
02cc376
Compare
Description
Related issue: #17976
This PR adds four built-in aggregation functions to the IoTDB table model:
rate(),increase(),irate(), anddelta(). Their calculation semantics are based on the corresponding Prometheus range-vector functions, while window boundaries follow IoTDB's[window_start, window_end)convention.Function semantics
raterate(value, time, window_start, window_end)increaseincrease(value, time, window_start, window_end)irateirate(value, time)deltadelta(value, time, window_start, window_end)The supported value types are
INT32,INT64,FLOAT, andDOUBLE. Time and window arguments acceptTIMESTAMPandINT64, withINT64interpreted using the currenttimestamp_precision. All four functions returnDOUBLE.rate(),increase(), anddelta()use all valid samples in the aggregation group and extrapolate the result to the window boundaries. Extrapolation uses the average sampling interval, a1.1threshold for distant boundaries, and counter zero-point protection where applicable.irate()only uses the last two valid samples and does not perform boundary extrapolation.Accumulator and planner design
Both
TableAccumulatorandGroupedAccumulatorexecution paths are supported.Each function has its own abstract accumulator class for shared function-specific validation, window handling, and result calculation. Ordered and naive accumulators are implemented as separate concrete classes because their state structures and lifecycle behavior differ significantly.
SINGLEaggregation step when the physical planner proves that the function'stimeargument is ascending within every SQL aggregation group. They consume samples incrementally without buffering and sorting all input rows.BLOBintermediate state containing the complete sample state and window boundaries.The ordered-input property is propagated through
AggregationNodeserialization so that distributed execution selects the same accumulator implementation as the coordinator plan.Validation and edge cases
The implementation handles the following cases:
NULLvalues are ignored.NULL.rate(),increase(), andirate().delta().NaNand positive or negative infinity, are rejected.NULLfor a valid sample.window_start < window_end.window_start <= time < window_end.Tests
Integration tests were added to
IoTDBTableAggregationITfor:The four target test methods were verified with both the
TableSimpleITandTableClusterITprofiles. The full reactor test compilation also passed for both the default and Chinese locales.This PR has:
Key changed/added classes (or packages if there are too many classes) in this PR
org.apache.iotdb.calc.execution.operator.source.relational.aggregation.rateorg.apache.iotdb.calc.execution.operator.source.relational.aggregation.grouped.rateAccumulatorFactoryTableMetadataImplTableDistributedPlanGeneratorAggregationNodePushAggregationIntoTableScanIoTDBTableAggregationIT