Repository navigation
Fix unreachable ordered rate accumulators - #19250
Conversation
JackieTien97
left a comment
There was a problem hiding this comment.
Thanks for working on this. I reviewed the planner change and verified it locally, and I didn't find a correctness problem: the per-group ordering argument (a single device, or grouping keys that cover every TAG column of a single-region DeviceTableScanNode) holds, and the remaining cases fall back to the naive accumulators.
What I checked:
- Real distributed plans built with
TestMetadata: a single device, and two devices in one region grouped by all TAG columns, are marked ordered;GROUP BY tag1, a global aggregation over two devices, DESC scans, and plans split into PARTIAL/FINAL are not. - A local 1C1D run with one DataRegion and four devices, with data in sequence files, unsequence files, the memtable, and two time partitions. Every rate-family query returned identical results with the native
timecolumn (ordered path) and with an equal INT64 column (naive path).jcmd VM.classesshowed that only theOrdered*accumulators ran for the multi-device query grouped by all TAG columns.
The inline comments are mostly about tests: the added tests don't actually protect the new planner wiring. There is also a time-argument-index conflict with #19227.
CI has not run on this PR yet.
JackieTien97
left a comment
There was a problem hiding this comment.
Thanks for the quick update. I re-checked f86deb9, and all six points from my previous review are addressed:
- The planner tests now build a SINGLE aggregation over a two-device scan, and
planSingleAggregationonly accepts SINGLE nodes. The mutant that treats every SINGLE aggregation as time ordered, which passed all tests before, now failstestTwoDevicesGroupedBySubsetOfTagsKeepNaiveInputandtestGlobalAggregationOverTwoDevicesKeepsNaiveInput. getOrderedInputTimeArgumentIndexis shared by both checks, so whichever of this PR and #19227 lands second only needs to register INTEGRAL (index 2) there.- The four-argument functions, the time alias, the wrapped
Sort/Limit/Collectchildren, and the excluded scan types are covered now, andTagColumnUtilsreplaces the three copies. TheParallelizeGroupingandTransformSortToStreamSortchanges look behavior-preserving to me.
Locally, 131 related tests pass: the Rate* tests plus ParallelizeGroupingTest, OrderingPropertyPropagationTest, TableFunctionSortTest, SortTest, SubQueryTest, LimitOffsetPushDownTest, and InsertIntoQueryTest. spotless:check also passes on the six changed files.
One small test issue is inline. A few other notes:
- Could you update the PR description? It still describes the first version of the tests and doesn't mention that
ParallelizeGroupingandTransformSortToStreamSortnow useTagColumnUtils. - An IT with several devices in one region grouped by all TAG columns would still be nice to have, but I don't consider it blocking.
- CI hasn't run on this PR yet.
Description
Problem
For typical rate aggregation plans whose child ordering does not include the rate function's time argument,
inputOrderedByTimeAscendingremainedfalse. These plans therefore used the naive accumulator, which buffers each group's(time, value)samples. The existing ordering-based path could still select an ordered accumulator when the child ordering proved the required order.Change
Extend the existing ordering-based check with a scan-based ordering guarantee for a single-region table scan.
For a
SINGLEaggregation over an ascending plain table scan, the scan input is considered time ordered per group when only order-preservingProjectorFilternodes lie between the aggregation and scan, and either:The rate function's second argument must also resolve through projections to the scan's native
timecolumn. Rate functions may use anotherINT64orTIMESTAMPcolumn as that argument, whose values are not necessarily ordered with the scan.This supplements the existing ordering-based check, preserving its behavior when the plan itself proves the required ordering.
Verification
RateOrderedInputTest: checks single-device input, all-TAG and subset grouping, descending scans, non-SINGLEsteps, projection resolution, and rejection of unsupported child plans.RateOrderedPlannerTest: checks the planner flag for single-device, multi-device, and non-time-argument queries.SELECT irate(s1, time) FROM t WHERE device_id = 'd1'constructsOrderedIrateAccumulator; the multi-device query constructsNaiveIrateAccumulator.