Add PERCENTILE aggregation function and related validation by FearfulTomcat27 · Pull Request #16545 · apache/iotdb

FearfulTomcat27 · 2025-10-09T14:24:49Z

This pull request introduces the new percentile aggregation function to the IoTDB relational query engine, providing support for calculating percentiles over numeric columns. The changes include the implementation of the core percentile calculation logic, integration into the accumulator factory, and comprehensive tests for both correct behavior and error handling.

Percentile Aggregation Support

Added PercentileAccumulator and supporting Percentile class to implement percentile calculations for numeric types (INT32, INT64, FLOAT, DOUBLE, TIMESTAMP). [1] [2]
Integrated the new percentile accumulator into the AccumulatorFactory, enabling both grouped and non-grouped percentile aggregations. [1] [2] [3]

Testing and Validation

Added new integration tests to IoTDBTableAggregationIT.java for verifying percentile queries and their results, including grouped queries.
Extended exception tests to cover invalid usages of the percentile function, such as wrong argument count, invalid percentage values, and unsupported data types.

Copilot

Pull request overview

This PR adds an exact percentile aggregation to the IoTDB relational query engine and wires it through parsing/semantic validation, aggregation execution (grouped + non-grouped), RPC aggregation type enums, and integration tests.

Changes:

Introduces a new exact percentile state container (Percentile) plus grouped/non-grouped accumulators.
Registers PERCENTILE across SQL constants, builtin function enums, semantic validation, and accumulator factory creation.
Adds integration tests for correct percentile results and invalid-usage error handling.

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 9 comments.

Show a summary per file

File	Description
`iotdb-protocol/thrift-commons/src/main/thrift/common.thrift`	Adds `PERCENTILE` to `TAggregationType` for RPC/planner/executor integration.
`iotdb-core/node-commons/src/main/java/org/apache/iotdb/commons/udf/builtin/relational/TableBuiltinAggregationFunction.java`	Registers `percentile` as a builtin aggregation and sets intermediate type handling.
`iotdb-core/datanode/src/main/java/org/apache/iotdb/db/utils/constant/SqlConstant.java`	Adds the `PERCENTILE` SQL function name constant.
`iotdb-core/datanode/src/main/java/org/apache/iotdb/db/queryengine/plan/relational/sql/parser/AstBuilder.java`	Adds parser-time validation for `percentile` argument literal type.
`iotdb-core/datanode/src/main/java/org/apache/iotdb/db/queryengine/plan/relational/metadata/TableMetadataImpl.java`	Adds semantic validation and return-type inference for `percentile`.
`iotdb-core/datanode/src/main/java/org/apache/iotdb/db/queryengine/execution/operator/source/relational/aggregation/grouped/array/PercentileBigArray.java`	Adds grouped state container for percentile states with retained-size tracking.
`iotdb-core/datanode/src/main/java/org/apache/iotdb/db/queryengine/execution/operator/source/relational/aggregation/grouped/GroupedPercentileAccumulator.java`	Implements grouped exact percentile aggregation accumulation/merge/finalization.
`iotdb-core/datanode/src/main/java/org/apache/iotdb/db/queryengine/execution/operator/source/relational/aggregation/PercentileAccumulator.java`	Implements non-grouped exact percentile aggregation accumulation/merge/finalization.
`iotdb-core/datanode/src/main/java/org/apache/iotdb/db/queryengine/execution/operator/source/relational/aggregation/AccumulatorFactory.java`	Wires `TAggregationType.PERCENTILE` to the new accumulators.
`iotdb-core/datanode/src/main/java/org/apache/iotdb/db/queryengine/execution/operator/source/relational/Percentile.java`	Adds the in-memory exact percentile computation + (de)serialization logic.
`integration-test/src/test/java/org/apache/iotdb/relational/it/query/recent/IoTDBTableAggregationIT.java`	Adds integration tests for `percentile` and invalid-usage error cases.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-23T17:20:38Z

+      for (int i = 0; i < positionCount; i++) {
+        position = selectedPositions[i];
+        groupId = groupIds[position];


When a mask is applied, selectedPositions may contain fewer valid entries than mask.getPositionCount(). The loop over selectedPositions should be bounded by mask.getSelectedPositionCount() to avoid aggregating masked-out rows (or stale positions).

Copilot · 2026-04-23T17:20:39Z

+      for (int i = 0; i < positionCount; i++) {
+        position = selectedPositions[i];
+        groupId = groupIds[position];


In the masked (non-selectAll) branch, iterate selectedPositions using mask.getSelectedPositionCount() as the loop bound to avoid aggregating stale/unselected positions.

Copilot · 2026-04-23T17:20:39Z

+    Percentile percentile = new Percentile();
+    if (size > percentile.capacity) {
+      percentile.capacity = size;
+      percentile.values = new double[size];
+    }


Percentile.deserialize() does not restore the 'sorted' state. Since serialize() writes values in their current internal order (which may be unsorted), leaving the deserialized instance with sorted=true can lead to incorrect getPercentile() results if the deserialized object is queried directly. Set sorted=false on deserialization (or serialize in sorted order) to preserve correctness.

Copilot · 2026-04-23T17:20:39Z

+  public void updateRetainedSize(long index, Percentile value) {
+    Percentile percentile = array.get(index);
+    if (percentile != null) {
+      sizeOfPercentile -= percentile.getEstimatedSize();
+    }


PercentileBigArray tracks retained size only when an element is set, but Percentile instances can grow their internal arrays as values are added. Since addValue()/merge() mutate the object after retrieval without calling updateRetainedSize, sizeOfPercentile can become arbitrarily stale and under-report memory. Consider updating the retained-size accounting when a Percentile grows (or redesign size tracking to avoid cached sizes for growable objects).

Copilot · 2026-04-23T17:20:39Z

+    array.forEach(
+        item -> {
+          if (item != null) {
+            item.clear();
+          }
+        });
+    sizeOfPercentile = 0;


reset() sets sizeOfPercentile = 0 even though the Percentile objects (and their backing arrays) remain allocated. This makes sizeOf()/isEmpty() inaccurate and can cause severe underestimation of operator memory usage after reset. Keep sizeOfPercentile consistent with retained objects (e.g., don't zero it, or recompute it based on current capacities).

Suggested change

array.forEach(

item -> {

if (item != null) {

item.clear();

}

});

sizeOfPercentile = 0;

sizeOfPercentile = 0;

array.forEach(

item -> {

if (item != null) {

item.clear();

sizeOfPercentile += item.getEstimatedSize();

}

});

Copilot · 2026-04-23T17:20:40Z

+    if (mask.isSelectAll()) {
+      for (int i = 0; i < positionCount; i++) {
+        if (!column.isNull(i)) {
+          percentile.addValue(column.getLong(i));
+        }


INT64/TIMESTAMP inputs are being implicitly converted to double when stored in Percentile, which silently loses precision for large INT64 values (>|2^53|) and can return incorrect results. Consider implementing a long-backed path for INT64/TIMESTAMP or using an exact conversion check (and throwing) when the long value cannot be represented exactly as a double.

Copilot · 2026-04-23T17:20:40Z

+      for (int i = 0; i < positionCount; i++) {
+        position = selectedPositions[i];
+        groupId = groupIds[position];


The non-selectAll path should iterate up to mask.getSelectedPositionCount() (not mask.getPositionCount()) when traversing selectedPositions; otherwise masked-out rows can be included.

Copilot · 2026-04-23T17:20:40Z

+      for (int i = 0; i < positionCount; i++) {
+        position = selectedPositions[i];
+        groupId = groupIds[position];


Looping over selectedPositions should use mask.getSelectedPositionCount() as the bound; using mask.getPositionCount() can include masked-out positions and corrupt results.

Copilot · 2026-04-23T17:20:41Z

+        int groupId = groupIds[i];
+        Percentile percentile = array.get(groupId);
+        if (!valueColumn.isNull(i)) {
+          percentile.addValue(valueColumn.getLong(i));
+        }


INT64/TIMESTAMP values are implicitly converted to double when stored (via Percentile.addValue(double)). This silently loses precision for values outside the exact double integer range (>|2^53|), producing incorrect percentiles. Consider storing longs separately for INT64/TIMESTAMP or, at minimum, enforce exact conversion (similar to AbstractApproxPercentileAccumulator.toDoubleExact) and fail fast when precision would be lost.

JackieTien97

感谢贡献！这个 PR 为 table model 增加了精确的 percentile 聚合函数，整体结构清晰、与现有 approx_percentile 的代码风格对齐，校验逻辑（参数个数 / 类型 / double literal）和正反向测试都比较完整。把 MemoryReservationManager 串进 AccumulatorFactory 各调用点的改动也是对的方向。

下面按严重程度给出几处需要处理的问题（详见行内评论）：

🔴 必须修复（正确性）

GroupedPercentileAccumulator 的 4 个 add*Input 在非 selectAll 分支用 mask.getPositionCount() 作循环边界，应为 getSelectedPositionCount()。带 FILTER / mask 的分组 percentile 会读到无效 position，导致结果错误甚至 AIOOBE。非分组版与现有 Mode/Avg accumulator 都用的是 getSelectedPositionCount()。

🟠 建议修复

PercentileBigArray.sizeOf() 内存核算被严重低估：sizeOfPercentile 只在分组首次创建时记一次初始容量，扩容后从不更新，使本 PR 的内存预留逻辑基本失效（大分组无法触发反压 / 防 OOM）。
新增 4 个文件用的是简化版 License 头，与全仓库统一的 ASF 长头不一致，license-maven-plugin 校验很可能让 CI 失败。

🟡 / 🔵 可一并处理

Percentile.deserialize 未重置 sorted 标志（当前靠 merge 兜底，属隐患）。
精确实现保留并传输全部原始值，getSerializedSize() 等存在整型溢出与大基数 OOM 风险，建议至少用 (long) 计算并在文档/实现上明确与 approx_percentile 的取舍。

测试建议

补 percentile(...) FILTER (WHERE ...) 的分组用例（可暴露上面 🔴 的问题）；
补一个会触发两阶段 partial→final 合并的分布式/多 region 用例（覆盖 serialize/deserialize/merge 路径）；
可选：大分组内存用例，验证内存预留生效。

其中 🔴 与 License 头两项建议合入前先解决。其余可按优先级跟进。

…andling

…r for percentile aggregation

… for improved memory management

…, memory accounting, license headers, and integer overflow 1. Percentile.deserialize now resets sorted=false to guarantee re-sort 2. PercentileBigArray.reset now recomputes sizeOfPercentile instead of zeroing 3. Fix Apache License header format in 4 new files (ASF full header) 4. Use long arithmetic in getSerializedSize and evaluateIntermediate to avoid overflow

)

FearfulTomcat27 force-pushed the feat/percentile branch from d18b6dd to f63a796 Compare April 23, 2026 17:12

Copilot AI review requested due to automatic review settings April 23, 2026 17:12

Copilot started reviewing on behalf of FearfulTomcat27 April 23, 2026 17:13 View session

Copilot AI reviewed Apr 23, 2026

View reviewed changes

FearfulTomcat27 force-pushed the feat/percentile branch from f63a796 to ce8a0ef Compare May 7, 2026 15:35

FearfulTomcat27 force-pushed the feat/percentile branch from ce8a0ef to 87cc668 Compare May 19, 2026 19:11

JackieTien97 reviewed Jun 6, 2026

View reviewed changes

FearfulTomcat27 force-pushed the feat/percentile branch from 87cc668 to 5b1c70c Compare June 10, 2026 19:58

FearfulTomcat27 added 6 commits June 12, 2026 10:15

feat: Add PERCENTILE aggregation function with validation and error h…

8a95cf6

…andling

feat: Implement GroupedPercentileAccumulator and PercentileAccumulato…

9529f0d

…r for percentile aggregation

feat: Integrate MemoryReservationManager into percentile accumulators…

ea80911

… for improved memory management

format: format using spotless

5c21459

fix: Fixing issues encountered during the conflict resolution process

c9e8d57

FearfulTomcat27 force-pushed the feat/percentile branch from 781f906 to c9e8d57 Compare June 12, 2026 02:27

JackieTien97 approved these changes Jun 12, 2026

View reviewed changes

JackieTien97 merged commit aec7dfb into apache:master Jun 12, 2026
41 checks passed

FearfulTomcat27 deleted the feat/percentile branch June 12, 2026 08:12

MileaRobertStefan pushed a commit to MileaRobertStefan/iotdb that referenced this pull request Jun 26, 2026

Add PERCENTILE aggregation function and related validation (apache#16545

a57a018

)

Uh oh!

Conversation

FearfulTomcat27 commented Oct 9, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Apr 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 23, 2026

Choose a reason for hiding this comment

Uh oh!

JackieTien97 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants