xiangfu0 commented on code in PR #19821:
URL: https://github.com/apache/pinot/pull/19821#discussion_r4278283843
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/operands/FunctionOperand.java:
##########
@@ -106,6 +110,8 @@ public FunctionOperand(RexExpression.FunctionCall
functionCall, DataSchema dataS
_operands.add(TransformOperandFactory.getTransformOperand(operand,
dataSchema));
}
_reusableOperandHolder = new Object[numOperands];
+ _cacheable = functionInfo.getVolatility() != FunctionVolatility.VOLATILE
+ && _operands.stream().allMatch(LiteralOperand.class::isInstance);
Review Comment:
Good call gating on `getVolatility()` rather than `isDeterministic()` here —
that's what lets non-foldable immutable functions still be cached, and
`testImmutableNonFoldableMetadata` covers exactly that distinction. One small
thing: on an empty operand list `allMatch` returns `true`, so zero-arg
immutable/stable calls are cacheable, which is the intent
(`operandCacheZeroArgs`). Fine as-is, just confirming it's deliberate.
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/operands/FunctionOperand.java:
##########
@@ -130,7 +139,12 @@ public Object apply(List<Object> row) {
}
result = _functionInvoker.invoke(_reusableOperandHolder);
}
- return result != null ?
TypeUtils.convert(_functionInvokerResultType.toInternal(result),
+ Object convertedResult = result != null ?
TypeUtils.convert(_functionInvokerResultType.toInternal(result),
_resultType.getStoredType()) : null;
+ if (_cacheable) {
+ _cachedResult = convertedResult;
+ _resultCached = true;
+ }
+ return convertedResult;
Review Comment:
Nice that the Javadoc calls out the shared read-only result contract. Worth
noting the new behavior for array/object results: before this change each row
got a freshly computed object, whereas now the same instance is returned for
every row. I verified this is safe today — `RowHeapDataBlock` documents array
cells as read-only downstream (only OBJECT aggregation cells get mutated, and
those are deep-copied), and `LiteralOperand` already returns a shared array for
SQL array literals. So the precedent holds; just flagging that accidental
downstream mutation would now corrupt all rows instead of one, should that
invariant ever change.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
