Repository navigation
Conversation
JackieTien97
requested changes
Nov 12, 2026
JackieTien97
left a comment
Contributor
There was a problem hiding this comment.
Thanks for the PR. I built this head (046bfcb) and its merge-base (2075032), ran both locally as 1C1D with default_data_region_group_num_per_database=4, and checked the three functions against an independent reference implementation.
What works
integral()values are correct whenever the query runs. A differential test against a Python trapezoid reference covered 3 random seeds and 9,014 result rows with 0 mismatches. It included DOUBLE and INT32 values, NULLs, negatives, 3 time partitions,date_binwindows, time filters, single-device queries and running windows.DERIVATIVEmatched on 2,354 random rows with 0 mismatches.SAMPLEreturns exactlymin(N, rows)per partition and is uniform: χ² = 3.8 / 3.6 / 2.5 (df = 4) over 2,000 partitions.- Not caching input slices in
TableFunctionOperatorwhen there is no pass-through output is correct. - The PR's own tests pass:
TableFunctionTest21/21,TableFunctionOperatorTest5/5, andIoTDBWindowTVFIT56/56 under both TableSimpleIT and TableClusterIT (1C3D). The changed non-i18n Java files pass google-java-format.
Blocking (details inline)
integral()fails on multi-region data once a group has more than 8,191 points in one region, because the PARTIAL state carries every raw sample. A plainGROUP BY device_idfails at 9,000 points per device. The ordered O(1) accumulators are never chosen by common plans.- This conflicts semantically with #19155, which is already on master. A merge into master
6e7448d9beis textually clean, but 6TableFunctionTestcases from #19155 then fail. NON_SPLITTABLE_TABLE_FUNCTIONSalso removes the per-region split for shapes that were already split safely. M4, SESSION and VARIATION become 1.7–2.7× slower on a single node. The prefix-partition bug it targets is real, but it should be fixed inParallelizeGrouping/canSplitTableFunctionProcessor, which would also cover user-defined TVFs.- The syntax differs from the agreed requirements, and the two new functions differ from each other:
integralaccepts only'1s', whileDERIVATIVEaccepts only1s.DERIVATIVEhas noTIMECOLand rejectsDESC.integraltakes its time column as a reference rather than a string.COLUMNS(*)does not work withintegral.
Also needs attention
- The
SAMPLEreservoir is unbounded and not memory-accounted: heap grows by about 890 MB forN => 4000000. - These semantics need agreement and documentation: NaN/Infinity, duplicate timestamps (results are currently non-deterministic), bare numeric time units, and NULL pairs in
DERIVATIVE. integral()used as a window function is O(n²).- Tests:
integral()has no UT or IT at all. That covers the accumulators, the state codec, the planner flag, the analyzer sugar, the PARTIAL/FINAL path and the error paths. Please add counterparts of the rate family's tests (RateAccumulatorFactoryTest,RateBatchInputTest,TableDistributedPlanGeneratorRateTest,AggregationNodeRateSerdeTest). Please also add an IT with more than 8,191 points per group, and planner tests for the TVF split change. - The PR description needs, per AGENTS.md, concrete example queries with their outputs for the three functions and a concrete reproduction for each bug fix. It should also mention these user-visible side changes:
- the new public udf-api
OrderByRequirement - the changed M4/FFT error text
- INT32 scalar arguments now raising an error on overflow instead of truncating (this affects FORECAST and user-defined TVFs)
- the
TableFunctionOperatorcaching change
- the new public udf-api
CI has not run on this PR yet; all workflows are waiting for approval.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds three built-in relational functions.
SAMPLE(DATA => table, N => n)
Table function that performs reservoir sampling over whole rows, with n rows
per partition. The first n rows fill the reservoir; each following row replaces
a uniformly chosen slot with probability n / (rows seen so far). All input
columns are kept, partition columns first, and the sampled rows are emitted in
input order.
DERIVATIVE(DATA => table PARTITION BY ... ORDER BY time, [unit => duration])
Table function that emits one output row per pair of adjacent points in ORDER
BY time within each partition. The output time is the time of the second point,
and each calculation column c outputs (c2 - c1) / (t2 - t1) * unit.
unitdefaults to 1s in the current timestamp precision; a null on either side only
makes that column null for the pair.
INTEGRAL(value [, time_unit [, time]])
Aggregate function that computes the trapezoidal integral of
valueovertime, divided bytime_unit.time_unitdefaults to 1s andtimedefaultsto the time column of the table. Ordered and naive accumulators are provided for
both grouped and ungrouped aggregation; the naive ones support intermediate
states so partial results can be merged in distributed aggregation.
Bug fixes
Fixes an existing planning bug for table functions that must run once per
partition (
mergeChildrenForSingleProcessorinTableDistributedPlanGenerator):common to all children, or by plain concatenation when there was none. The
merged stream was therefore not ordered by the GROUP clause (partition keys
followed by the in-partition ordering), and rows of one partition could arrive
non-consecutively when a partition spans several children, e.g. PARTITION BY
on non-device columns or a device spanning regions. Such a partition was then
treated as several partitions. The merged input now uses the ordering of the
GroupNode and inserts an explicit SortNode when the children do not already
provide it.
canSplitTableFunctionProcessoronly excluded FFT from per-child splitting.M4, FORECAST, PATTERN_MATCH, CLASSIFY, LOWPASS, HIGHPASS, XCORR, SESSION,
VARIATION and CAPACITY could be cloned for every child, so one logical
partition could be handled by several processors with separate state. They are
now listed in
NON_SPLITTABLE_TABLE_FUNCTIONS.