Copilot commented on code in PR #18333:
URL: https://github.com/apache/iotdb/pull/18333#discussion_r3681452303
##########
iotdb-core/calc-commons/src/main/java/org/apache/iotdb/calc/execution/operator/process/function/TableFunctionOperator.java:
##########
@@ -281,18 +411,18 @@ public void close() throws Exception {
@Override
public boolean isFinished() throws Exception {
- return finished;
+ return finished && resultTsBlocks.isEmpty();
}
@Override
public long calculateMaxPeekMemory() {
return inputOperator.calculateMaxPeekMemory()
- + Math.max(DEFAULT_MAX_TSBLOCK_SIZE_IN_BYTES,
properBlockBuilder.getRetainedSizeInBytes());
+ + Math.max(maxTsBlockSizeInBytes,
properBlockBuilder.getRetainedSizeInBytes());
}
@Override
public long calculateMaxReturnSize() {
- return Math.max(DEFAULT_MAX_TSBLOCK_SIZE_IN_BYTES,
properBlockBuilder.getRetainedSizeInBytes());
+ return maxTsBlockSizeInBytes;
}
Review Comment:
`calculateMaxReturnSize()` always returns `maxTsBlockSizeInBytes`, but the
splitting logic can still return a 1-row region whose estimated size exceeds
the max (when a single row itself is larger than the configured limit).
Returning a constant max risks under-estimating peak per-`next()` output size
for memory scheduling.
##########
iotdb-core/calc-commons/src/main/java/org/apache/iotdb/calc/execution/operator/process/function/TableFunctionOperator.java:
##########
@@ -241,16 +252,134 @@ private List<TsBlock> buildTsBlock(
int subBlockPositionCount = passThroughColumns[0].getPositionCount();
TsBlock subProperBlock = properBlock.getRegion(builtCount,
subBlockPositionCount);
builtCount += subBlockPositionCount;
- result.add(subProperBlock.appendValueColumns(passThroughColumns));
+ addSplitTsBlocks(result,
subProperBlock.appendValueColumns(passThroughColumns));
}
} else {
- // split the proper block into smaller blocks
- result.add(properBlock);
+ addSplitTsBlocks(result, properBlock);
}
properBlockBuilder.reset();
return result;
}
+ /**
+ * Splits the final result using the same logical in-memory size accounting
as {@link
+ * TsBlockBuilder}.
+ *
+ * <p>Serializing candidate regions to find their exact sizes would write
every value into
+ * temporary buffers, only for the exchange layer to serialize the selected
regions again.
+ * Rebuilding the result with a size-tracking {@link TsBlockBuilder} would
avoid that temporary
+ * serialization, but it would turn the UDF's batched column output into
row-by-row,
+ * column-by-column copies.
+ *
+ * <p>Instead, fixed-width values are accounted for directly from their data
types, while only the
+ * retained sizes of variable-width values are inspected. This deliberately
estimates the
+ * in-memory TsBlock size rather than its serialized size because the two
representations are not
+ * equivalent. The resulting regions are views over the original columns and
do not copy their
Review Comment:
The PR description says `buildTsBlock()` splits by *exact serialized size*
using `TsBlockSerde` and uses binary search to find a fitting row prefix. The
implementation here explicitly avoids serialization and instead splits by an
*estimated in-memory* size model (see the method Javadoc and
`getEstimatedPositionSizeInBytes`). Please reconcile this: either update the PR
description/acceptance criteria to match the in-memory estimation approach, or
adjust the implementation to enforce the serialized-size bound as described.
##########
iotdb-core/calc-commons/src/main/java/org/apache/iotdb/calc/execution/operator/process/function/TableFunctionOperator.java:
##########
@@ -62,14 +64,15 @@ public class TableFunctionOperator implements
ProcessOperator {
private static final long INSTANCE_SIZE =
RamUsageEstimator.shallowSizeOfInstance(AggregationMergeSortOperator.class);
Review Comment:
`INSTANCE_SIZE` is calculated using `AggregationMergeSortOperator.class`,
which makes `ramBytesUsed()` for `TableFunctionOperator` incorrect and can skew
memory accounting. This looks like a copy/paste mistake; other operators use
their own class when calling `RamUsageEstimator.shallowSizeOfInstance(...)`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
