Skip to content

Commit 5b1c70c

Browse files
Fix review issues for PERCENTILE aggregation: deserialize sorted flag, 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
1 parent d016239 commit 5b1c70c

4 files changed

Lines changed: 113 additions & 66 deletions

File tree

iotdb-core/calc-commons/src/main/java/org/apache/iotdb/calc/execution/operator/source/relational/Percentile.java

Lines changed: 21 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1,15 +1,20 @@
11
/*
2-
* Licensed under the Apache License, Version 2.0 (the "License");
3-
* you may not use this file except in compliance with the License.
4-
* You may obtain a copy of the License at
2+
* Licensed to the Apache Software Foundation (ASF) under one
3+
* or more contributor license agreements. See the NOTICE file
4+
* distributed with this work for additional information
5+
* regarding copyright ownership. The ASF licenses this file
6+
* to you under the Apache License, Version 2.0 (the
7+
* "License"); you may not use this file except in compliance
8+
* with the License. You may obtain a copy of the License at
59
*
610
* http://www.apache.org/licenses/LICENSE-2.0
711
*
8-
* Unless required by applicable law or agreed to in writing, software
9-
* distributed under the License is distributed on an "AS IS" BASIS,
10-
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
11-
* See the License for the specific language governing permissions and
12-
* limitations under the License.
12+
* Unless required by applicable law or agreed to in writing,
13+
* software distributed under the License is distributed on an
14+
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
15+
* KIND, either express or implied. See the License for the
16+
* specific language governing permissions and limitations
17+
* under the License.
1318
*/
1419

1520
package org.apache.iotdb.calc.execution.operator.source.relational;
@@ -104,6 +109,12 @@ public int getSize() {
104109
}
105110

106111
public void clear() {
112+
// Shrink the backing array back to the initial capacity so the memory held by a large group is
113+
// actually released on reset, instead of staying reserved at the historical peak capacity.
114+
if (capacity > INITIAL_CAPACITY) {
115+
capacity = INITIAL_CAPACITY;
116+
values = new double[capacity];
117+
}
107118
size = 0;
108119
sorted = true;
109120
}
@@ -147,11 +158,12 @@ public static Percentile deserialize(ByteBuffer buffer) {
147158
for (int i = 0; i < size; i++) {
148159
percentile.values[i] = ReadWriteIOUtils.readDouble(buffer);
149160
}
161+
percentile.sorted = false;
150162
return percentile;
151163
}
152164

153165
public int getSerializedSize() {
154-
return Integer.BYTES + (size * Double.BYTES);
166+
return Integer.BYTES + (int) ((long) size * Double.BYTES);
155167
}
156168

157169
public long getEstimatedSize() {

iotdb-core/calc-commons/src/main/java/org/apache/iotdb/calc/execution/operator/source/relational/aggregation/PercentileAccumulator.java

Lines changed: 30 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1,15 +1,20 @@
11
/*
2-
* Licensed under the Apache License, Version 2.0 (the "License");
3-
* you may not use this file except in compliance with the License.
4-
* You may obtain a copy of the License at
2+
* Licensed to the Apache Software Foundation (ASF) under one
3+
* or more contributor license agreements. See the NOTICE file
4+
* distributed with this work for additional information
5+
* regarding copyright ownership. The ASF licenses this file
6+
* to you under the Apache License, Version 2.0 (the
7+
* "License"); you may not use this file except in compliance
8+
* with the License. You may obtain a copy of the License at
59
*
610
* http://www.apache.org/licenses/LICENSE-2.0
711
*
8-
* Unless required by applicable law or agreed to in writing, software
9-
* distributed under the License is distributed on an "AS IS" BASIS,
10-
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
11-
* See the License for the specific language governing permissions and
12-
* limitations under the License.
12+
* Unless required by applicable law or agreed to in writing,
13+
* software distributed under the License is distributed on an
14+
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
15+
* KIND, either express or implied. See the License for the
16+
* specific language governing permissions and limitations
17+
* under the License.
1318
*/
1419

1520
package org.apache.iotdb.calc.execution.operator.source.relational.aggregation;
@@ -35,7 +40,10 @@ public class PercentileAccumulator implements TableAccumulator {
3540

3641
private final TSDataType seriesDataType;
3742
private Percentile percentile = new Percentile();
43+
// percentage is a query-level constant; it is read once from the first input/intermediate and
44+
// kept fixed afterwards, so it never gets reset to 0 by a later all-null batch.
3845
private double percentage;
46+
private boolean percentageInitialized;
3947

4048
private final MemoryReservationManager memoryReservationManager;
4149
private long previousPercentileSize;
@@ -63,7 +71,10 @@ public void addInput(Column[] arguments, AggregationMask mask) {
6371
throw new SemanticException(
6472
String.format("PERCENTILE requires 2 arguments, but got %d", arguments.length));
6573
}
66-
percentage = arguments[1].getDouble(0);
74+
if (!percentageInitialized) {
75+
percentage = arguments[1].getDouble(0);
76+
percentageInitialized = true;
77+
}
6778
switch (seriesDataType) {
6879
case INT32:
6980
addIntInput(arguments[0], mask);
@@ -91,7 +102,13 @@ public void addIntermediate(Column argument) {
91102
if (!argument.isNull(i)) {
92103
byte[] data = argument.getBinary(i).getValues();
93104
ByteBuffer buffer = ByteBuffer.wrap(data);
94-
this.percentage = ReadWriteIOUtils.readDouble(buffer);
105+
// Always consume the leading 8 bytes so the buffer position is correct for deserialize,
106+
// but only keep the percentage once: every partial carries the same query-level constant.
107+
double serializedPercentage = ReadWriteIOUtils.readDouble(buffer);
108+
if (!percentageInitialized) {
109+
percentage = serializedPercentage;
110+
percentageInitialized = true;
111+
}
95112
percentile.merge(Percentile.deserialize(buffer));
96113
}
97114
}
@@ -101,7 +118,8 @@ public void addIntermediate(Column argument) {
101118
@Override
102119
public void evaluateIntermediate(ColumnBuilder columnBuilder) {
103120
int percentileDataLength = percentile.getSerializedSize();
104-
ByteBuffer buffer = ByteBuffer.allocate(8 + percentileDataLength);
121+
// Use long arithmetic to avoid integer overflow
122+
ByteBuffer buffer = ByteBuffer.allocate(Math.toIntExact(8L + percentileDataLength));
105123
ReadWriteIOUtils.write(percentage, buffer);
106124
percentile.serialize(buffer);
107125
columnBuilder.writeBinary(new Binary(buffer.array()));
@@ -147,6 +165,7 @@ public void addStatistics(Statistics[] statistics) {
147165
@Override
148166
public void reset() {
149167
percentile = new Percentile();
168+
percentageInitialized = false;
150169
updateMemoryReservation();
151170
}
152171

iotdb-core/calc-commons/src/main/java/org/apache/iotdb/calc/execution/operator/source/relational/aggregation/grouped/GroupedPercentileAccumulator.java

Lines changed: 34 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1,15 +1,20 @@
11
/*
2-
* Licensed under the Apache License, Version 2.0 (the "License");
3-
* you may not use this file except in compliance with the License.
4-
* You may obtain a copy of the License at
2+
* Licensed to the Apache Software Foundation (ASF) under one
3+
* or more contributor license agreements. See the NOTICE file
4+
* distributed with this work for additional information
5+
* regarding copyright ownership. The ASF licenses this file
6+
* to you under the Apache License, Version 2.0 (the
7+
* "License"); you may not use this file except in compliance
8+
* with the License. You may obtain a copy of the License at
59
*
610
* http://www.apache.org/licenses/LICENSE-2.0
711
*
8-
* Unless required by applicable law or agreed to in writing, software
9-
* distributed under the License is distributed on an "AS IS" BASIS,
10-
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
11-
* See the License for the specific language governing permissions and
12-
* limitations under the License.
12+
* Unless required by applicable law or agreed to in writing,
13+
* software distributed under the License is distributed on an
14+
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
15+
* KIND, either express or implied. See the License for the
16+
* specific language governing permissions and limitations
17+
* under the License.
1318
*/
1419

1520
package org.apache.iotdb.calc.execution.operator.source.relational.aggregation.grouped;
@@ -34,7 +39,10 @@ public class GroupedPercentileAccumulator implements GroupedAccumulator {
3439
private static final long INSTANCE_SIZE =
3540
RamUsageEstimator.shallowSizeOfInstance(GroupedPercentileAccumulator.class);
3641
private final TSDataType seriesDataType;
42+
// percentage is a query-level constant; it is read once from the first input/intermediate and
43+
// kept fixed afterwards, so it never gets reset to 0 by a later all-null batch.
3744
private double percentage;
45+
private boolean percentageInitialized;
3846
private final MemoryReservationManager memoryReservationManager;
3947
private long previousArraySize;
4048
private final PercentileBigArray array = new PercentileBigArray();
@@ -62,7 +70,10 @@ public void addInput(int[] groupIds, Column[] arguments, AggregationMask mask) {
6270
throw new SemanticException(
6371
String.format("PERCENTILE requires 2 arguments, but got %d", arguments.length));
6472
}
65-
percentage = arguments[1].getDouble(0);
73+
if (!percentageInitialized) {
74+
percentage = arguments[1].getDouble(0);
75+
percentageInitialized = true;
76+
}
6677

6778
switch (seriesDataType) {
6879
case INT32:
@@ -92,7 +103,13 @@ public void addIntermediate(int[] groupIds, Column argument) {
92103
if (!argument.isNull(i)) {
93104
byte[] data = argument.getBinary(i).getValues();
94105
ByteBuffer buffer = ByteBuffer.wrap(data);
95-
this.percentage = ReadWriteIOUtils.readDouble(buffer);
106+
// Always consume the leading 8 bytes so the buffer position is correct for deserialize,
107+
// but only keep the percentage once: every partial carries the same query-level constant.
108+
double serializedPercentage = ReadWriteIOUtils.readDouble(buffer);
109+
if (!percentageInitialized) {
110+
percentage = serializedPercentage;
111+
percentageInitialized = true;
112+
}
96113
Percentile other = Percentile.deserialize(buffer);
97114
array.get(groupId).merge(other);
98115
}
@@ -104,7 +121,8 @@ public void addIntermediate(int[] groupIds, Column argument) {
104121
public void evaluateIntermediate(int groupId, ColumnBuilder columnBuilder) {
105122
Percentile percentile = array.get(groupId);
106123
int percentileDataLength = percentile.getSerializedSize();
107-
ByteBuffer buffer = ByteBuffer.allocate(8 + percentileDataLength);
124+
// Use long arithmetic to avoid integer overflow
125+
ByteBuffer buffer = ByteBuffer.allocate(Math.toIntExact(8L + percentileDataLength));
108126
ReadWriteIOUtils.write(percentage, buffer);
109127
percentile.serialize(buffer);
110128
columnBuilder.writeBinary(new Binary(buffer.array()));
@@ -144,6 +162,7 @@ public void prepareFinal() {}
144162
@Override
145163
public void reset() {
146164
array.reset();
165+
percentageInitialized = false;
147166
updateMemoryReservation();
148167
}
149168

@@ -161,7 +180,7 @@ private void updateMemoryReservation() {
161180
public void addIntInput(int[] groupIds, Column[] arguments, AggregationMask mask) {
162181
Column valueColumn = arguments[0];
163182

164-
int positionCount = mask.getPositionCount();
183+
int positionCount = mask.getSelectedPositionCount();
165184

166185
if (mask.isSelectAll()) {
167186
for (int i = 0; i < positionCount; i++) {
@@ -189,7 +208,7 @@ public void addIntInput(int[] groupIds, Column[] arguments, AggregationMask mask
189208
public void addLongInput(int[] groupIds, Column[] arguments, AggregationMask mask) {
190209
Column valueColumn = arguments[0];
191210

192-
int positionCount = mask.getPositionCount();
211+
int positionCount = mask.getSelectedPositionCount();
193212

194213
if (mask.isSelectAll()) {
195214
for (int i = 0; i < positionCount; i++) {
@@ -217,7 +236,7 @@ public void addLongInput(int[] groupIds, Column[] arguments, AggregationMask mas
217236
public void addFloatInput(int[] groupIds, Column[] arguments, AggregationMask mask) {
218237
Column valueColumn = arguments[0];
219238

220-
int positionCount = mask.getPositionCount();
239+
int positionCount = mask.getSelectedPositionCount();
221240

222241
if (mask.isSelectAll()) {
223242
for (int i = 0; i < positionCount; i++) {
@@ -245,7 +264,7 @@ public void addFloatInput(int[] groupIds, Column[] arguments, AggregationMask ma
245264
public void addDoubleInput(int[] groupIds, Column[] arguments, AggregationMask mask) {
246265
Column valueColumn = arguments[0];
247266

248-
int positionCount = mask.getPositionCount();
267+
int positionCount = mask.getSelectedPositionCount();
249268

250269
if (mask.isSelectAll()) {
251270
for (int i = 0; i < positionCount; i++) {
Original file line numberDiff line numberDiff line change
@@ -1,15 +1,20 @@
11
/*
2-
* Licensed under the Apache License, Version 2.0 (the "License");
3-
* you may not use this file except in compliance with the License.
4-
* You may obtain a copy of the License at
2+
* Licensed to the Apache Software Foundation (ASF) under one
3+
* or more contributor license agreements. See the NOTICE file
4+
* distributed with this work for additional information
5+
* regarding copyright ownership. The ASF licenses this file
6+
* to you under the Apache License, Version 2.0 (the
7+
* "License"); you may not use this file except in compliance
8+
* with the License. You may obtain a copy of the License at
59
*
610
* http://www.apache.org/licenses/LICENSE-2.0
711
*
8-
* Unless required by applicable law or agreed to in writing, software
9-
* distributed under the License is distributed on an "AS IS" BASIS,
10-
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
11-
* See the License for the specific language governing permissions and
12-
* limitations under the License.
12+
* Unless required by applicable law or agreed to in writing,
13+
* software distributed under the License is distributed on an
14+
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
15+
* KIND, either express or implied. See the License for the
16+
* specific language governing permissions and limitations
17+
* under the License.
1318
*/
1419

1520
package org.apache.iotdb.calc.execution.operator.source.relational.aggregation.grouped.array;
@@ -22,55 +27,47 @@
2227
public final class PercentileBigArray {
2328
private static final long INSTANCE_SIZE = shallowSizeOfInstance(PercentileBigArray.class);
2429
private final ObjectBigArray<Percentile> array;
25-
private long sizeOfPercentile;
2630

2731
public PercentileBigArray() {
2832
array = new ObjectBigArray<>();
2933
}
3034

35+
/**
36+
* Unlike fixed-size sketches (e.g. TDigest), each {@link Percentile} stores all raw values and
37+
* grows unboundedly as values are appended through {@link #get(long)}. Caching the retained size
38+
* and only refreshing it on {@code set} would therefore drift far below the real footprint, so we
39+
* sum the live estimated size of every Percentile on demand to keep memory accounting accurate.
40+
*/
3141
public long sizeOf() {
32-
return INSTANCE_SIZE + shallowSizeOf(array) + sizeOfPercentile;
42+
long[] sizeOfPercentile = {0};
43+
array.forEach(
44+
item -> {
45+
if (item != null) {
46+
sizeOfPercentile[0] += item.getEstimatedSize();
47+
}
48+
});
49+
return INSTANCE_SIZE + shallowSizeOf(array) + sizeOfPercentile[0];
3350
}
3451

3552
public Percentile get(long index) {
3653
Percentile percentile = array.get(index);
3754
if (percentile == null) {
3855
percentile = new Percentile();
39-
set(index, percentile);
56+
array.set(index, percentile);
4057
}
4158
return percentile;
4259
}
4360

44-
public void set(long index, Percentile value) {
45-
updateRetainedSize(index, value);
46-
array.set(index, value);
47-
}
48-
49-
public boolean isEmpty() {
50-
return sizeOfPercentile == 0;
51-
}
52-
5361
public void ensureCapacity(long length) {
5462
array.ensureCapacity(length);
5563
}
5664

57-
public void updateRetainedSize(long index, Percentile value) {
58-
Percentile percentile = array.get(index);
59-
if (percentile != null) {
60-
sizeOfPercentile -= percentile.getEstimatedSize();
61-
}
62-
if (value != null) {
63-
sizeOfPercentile += value.getEstimatedSize();
64-
}
65-
}
66-
6765
public void reset() {
6866
array.forEach(
6967
item -> {
7068
if (item != null) {
7169
item.clear();
7270
}
7371
});
74-
sizeOfPercentile = 0;
7572
}
7673
}

0 commit comments

Comments
 (0)