Jackie-Jiang commented on code in PR #19008:
URL: https://github.com/apache/pinot/pull/19008#discussion_r3606115851
##########
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:
(minor) Use markdown style for javadoc, same for other places
##########
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:
This is the core bug. Fixing this should be enough
##########
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:
The `ArrayList` returned by `Arrays.asList()` is not the same as regular
`ArrayList`, and it doesn't support add(). Let's revert these changes and
always use regular `ArrayList` in the `Function`.
##########
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:
Let's do a `instanceof` check before making the copy
--
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]
