navina commented on code in PR #19008:
URL: https://github.com/apache/pinot/pull/19008#discussion_r3607041237
##########
pinot-broker/src/test/java/org/apache/pinot/broker/requesthandler/BaseSingleStageBrokerRequestHandlerTest.java:
##########
@@ -857,6 +861,148 @@ protected BrokerResponseNative
processMaterializedViewSplitBrokerRequest(long re
///
org.apache.pinot.materializedview.handler.DefaultMaterializedViewHandler#attachFilter;
their
/// regression coverage lives in DefaultMaterializedViewHandlerTest in
pinot-materialized-view.
+ /**
Review Comment:
Done — converted all the Javadoc I added (this block,
`RequestUtils.getFunction`, and `RequestUtilsTest`) to `///` markdown style.
##########
pinot-common/src/main/java/org/apache/pinot/sql/parsers/rewriter/RlsFiltersRewriter.java:
##########
@@ -69,7 +68,7 @@ public PinotQuery rewrite(PinotQuery pinotQuery) {
Expression existingFilterExpression = pinotQuery.getFilterExpression();
if (existingFilterExpression != null) {
Expression combinedFilterExpression =
- RequestUtils.getFunctionExpression(FilterKind.AND.name(),
List.of(expression, existingFilterExpression));
+ RequestUtils.getFunctionExpression(FilterKind.AND.name(),
expression, existingFilterExpression);
Review Comment:
Agreed — kept this as the core fix (switched to the varargs
`getFunctionExpression` overload). The `RequestUtils` change is now just a
lightweight safety net rather than the primary fix.
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/request/RequestUtils.java:
##########
@@ -487,22 +487,34 @@ public static String getLiteralString(Expression
expression) {
return getLiteralString(literal);
}
+ /**
+ * Creates a {@link Function} with the given operands. The returned
function's operand list is
+ * always a fresh mutable {@link ArrayList}, so a caller-supplied immutable
list (e.g.
+ * {@code List.of(...)}, {@code Arrays.asList(...)}, or a {@code
Stream.toList()} result) is
+ * defensively copied. This is a hard contract of this factory: many query
rewriters and filter
+ * optimizers mutate operands in place (e.g. via {@code
getOperands().replaceAll(...)},
+ * {@code set(...)}, or {@code add(...)}), and an immutable list would throw
+ * {@link UnsupportedOperationException} far from where it was created.
+ */
public static Function getFunction(String canonicalName, List<Expression>
operands) {
Function function = new Function(canonicalName);
- function.setOperands(operands);
+ // Defensive copy so the operand list is always mutable for downstream
in-place rewrites.
+ function.setOperands(new ArrayList<>(operands));
return function;
}
+ /**
+ * See {@link #getFunction(String, List)} for the mutable-operand-list
contract.
+ */
public static Function getFunction(String canonicalName, Expression operand)
{
- // NOTE: Create an ArrayList because we might need to modify the list later
- List<Expression> operands = new ArrayList<>(1);
- operands.add(operand);
- return getFunction(canonicalName, operands);
+ return getFunction(canonicalName, Arrays.asList(operand));
Review Comment:
Good catch — reverted both sibling overloads to their original form so a
regular `ArrayList` is always stored in the `Function` (no `Arrays.asList`
result kept as operands).
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/request/RequestUtils.java:
##########
@@ -487,22 +487,34 @@ public static String getLiteralString(Expression
expression) {
return getLiteralString(literal);
}
+ /**
+ * Creates a {@link Function} with the given operands. The returned
function's operand list is
+ * always a fresh mutable {@link ArrayList}, so a caller-supplied immutable
list (e.g.
+ * {@code List.of(...)}, {@code Arrays.asList(...)}, or a {@code
Stream.toList()} result) is
+ * defensively copied. This is a hard contract of this factory: many query
rewriters and filter
+ * optimizers mutate operands in place (e.g. via {@code
getOperands().replaceAll(...)},
+ * {@code set(...)}, or {@code add(...)}), and an immutable list would throw
+ * {@link UnsupportedOperationException} far from where it was created.
+ */
public static Function getFunction(String canonicalName, List<Expression>
operands) {
Function function = new Function(canonicalName);
- function.setOperands(operands);
+ // Defensive copy so the operand list is always mutable for downstream
in-place rewrites.
Review Comment:
Done — the `List` overload now does `operands instanceof ArrayList ?
operands : new ArrayList<>(operands)`, so it reuses the caller's list on the
common path and copies only when the input isn't already an `ArrayList` (e.g.
`List.of(...)`).
--
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]
