This is an automated email from the ASF dual-hosted git repository.
xiangfu0 pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/pinot.git
The following commit(s) were added to refs/heads/master by this push:
new dbc50f4cad5 Follow-up to #18941: routing-filter pruner-invariant test
+ construction cleanup (#18951)
dbc50f4cad5 is described below
commit dbc50f4cad5e64b5d3a02a00ea4ac3acf781fea2
Author: Xiang Fu <[email protected]>
AuthorDate: Fri Jul 10 20:39:10 2026 +0200
Follow-up to #18941: routing-filter pruner-invariant test + construction
cleanup (#18951)
* Follow-up to #18941: routing-filter test + construction cleanup
#18941 fixed segment pruners throwing "No enum constant
org.apache.pinot.sql.FilterKind.<op>" when a boolean scalar function is used
directly as a predicate, by wrapping non-FilterKind operators as
EQUALS(fn, true). This is a small follow-up on top of it.
Test: add testMixedPredicateYieldsOnlyFilterKindOperatorsForPruners, which
asserts the invariant pruners actually depend on -- every operator they
traverse
resolves via FilterKind.valueOf -- over a whole normalized filter tree
rather
than a single wrapped node. It uses the shape of a reported failure, where
arrayContainsString canonicalized to the lowercase operator
"arraycontainsstring" and sat directly under AND.
Cleanup: build the EQUALS/AND expressions via
RequestUtils.getFunctionExpression
instead of hand-rolling Expression/Function and their operand lists in three
places (addFilterExpression, the always-false literal branch, and
wrapAsEqualsTrue). RequestUtils.getFunction already allocates a mutable
ArrayList for operands, so this is behavior-preserving; it also drops the
now
unused ExpressionType import.
* Address review: assert pruner-visited nodes are FUNCTION expressions
Segment pruners dereference filterExpression.getFunctionCall()
unconditionally
before resolving the operator, so a non-FUNCTION node NPEs them -- which is
precisely what ensureFilterIsFunctionExpression exists to prevent. The
invariant
helper returned early on a null function call, so a regression producing a
non-FUNCTION node would have passed silently.
Assert the function call is non-null instead of returning, and broaden the
predicate under test to mix all the shapes that arrive non-FUNCTION or
non-FilterKind: a boolean scalar function, a bare boolean identifier, and a
constant-folded FALSE literal. Also assert no operand is dropped.
Verified the assertion has teeth: reverting the identifier-wrapping in
ensureFilterIsFunctionExpression makes the test fail on the bare IDENTIFIER
node rather than pass.
---
.../planner/logical/LeafStageToPinotQuery.java | 30 ++++++---------
.../planner/logical/LeafStageToPinotQueryTest.java | 44 ++++++++++++++++++++++
2 files changed, 55 insertions(+), 19 deletions(-)
diff --git
a/pinot-query-planner/src/main/java/org/apache/pinot/query/planner/logical/LeafStageToPinotQuery.java
b/pinot-query-planner/src/main/java/org/apache/pinot/query/planner/logical/LeafStageToPinotQuery.java
index f6777b9dcc3..c6fca972052 100644
---
a/pinot-query-planner/src/main/java/org/apache/pinot/query/planner/logical/LeafStageToPinotQuery.java
+++
b/pinot-query-planner/src/main/java/org/apache/pinot/query/planner/logical/LeafStageToPinotQuery.java
@@ -30,7 +30,6 @@ import org.apache.calcite.rel.core.TableScan;
import org.apache.commons.lang3.EnumUtils;
import org.apache.pinot.common.request.DataSource;
import org.apache.pinot.common.request.Expression;
-import org.apache.pinot.common.request.ExpressionType;
import org.apache.pinot.common.request.Function;
import org.apache.pinot.common.request.PinotQuery;
import org.apache.pinot.common.utils.request.RequestUtils;
@@ -137,11 +136,8 @@ public class LeafStageToPinotQuery {
pinotQuery.setFilterExpression(filterExpression);
return;
}
- Function andFunction = new Function(FilterKind.AND.name());
- andFunction.setOperands(new ArrayList<>(List.of(existingFilter,
filterExpression)));
- Expression combined = new Expression(ExpressionType.FUNCTION);
- combined.setFunctionCall(andFunction);
- pinotQuery.setFilterExpression(combined);
+ pinotQuery.setFilterExpression(
+ RequestUtils.getFunctionExpression(FilterKind.AND.name(),
existingFilter, filterExpression));
}
/**
@@ -218,24 +214,20 @@ public class LeafStageToPinotQuery {
// don't unnecessarily scan everything.
if (expression.getLiteral() != null &&
expression.getLiteral().isSetBoolValue()
&& !expression.getLiteral().getBoolValue()) {
- Function alwaysFalse = new Function(FilterKind.EQUALS.name());
- alwaysFalse.setOperands(new ArrayList<>(List.of(
- RequestUtils.getLiteralExpression(0),
- RequestUtils.getLiteralExpression(1))));
- Expression wrapped = new Expression(ExpressionType.FUNCTION);
- wrapped.setFunctionCall(alwaysFalse);
- return wrapped;
+ // EQUALS(0, 1) — a predicate that is always false.
+ return RequestUtils.getFunctionExpression(FilterKind.EQUALS.name(),
+ RequestUtils.getLiteralExpression(0),
RequestUtils.getLiteralExpression(1));
}
// LITERAL true or non-boolean literals have no filter constraint so we
skip pruning
return null;
}
- /** Wraps a boolean-valued expression as the standard predicate {@code
EQUALS(expression, true)}. */
+ /**
+ * Wraps a boolean-valued expression as the standard predicate {@code
EQUALS(expression, true)}. The operand list
+ * is mutable so downstream rewriters (e.g. {@code
PredicateComparisonRewriter}) can modify it.
+ */
private static Expression wrapAsEqualsTrue(Expression expression) {
- Function equalsFunction = new Function(FilterKind.EQUALS.name());
- equalsFunction.setOperands(new ArrayList<>(List.of(expression,
RequestUtils.getLiteralExpression(true))));
- Expression wrapped = new Expression(ExpressionType.FUNCTION);
- wrapped.setFunctionCall(equalsFunction);
- return wrapped;
+ return RequestUtils.getFunctionExpression(FilterKind.EQUALS.name(),
expression,
+ RequestUtils.getLiteralExpression(true));
}
}
diff --git
a/pinot-query-planner/src/test/java/org/apache/pinot/query/planner/logical/LeafStageToPinotQueryTest.java
b/pinot-query-planner/src/test/java/org/apache/pinot/query/planner/logical/LeafStageToPinotQueryTest.java
index 76ac02898af..da441625044 100644
---
a/pinot-query-planner/src/test/java/org/apache/pinot/query/planner/logical/LeafStageToPinotQueryTest.java
+++
b/pinot-query-planner/src/test/java/org/apache/pinot/query/planner/logical/LeafStageToPinotQueryTest.java
@@ -39,6 +39,7 @@ import org.apache.pinot.common.request.Function;
import org.apache.pinot.common.request.PinotQuery;
import org.apache.pinot.common.utils.request.RequestUtils;
import org.apache.pinot.query.type.TypeFactory;
+import org.apache.pinot.sql.FilterKind;
import org.mockito.Mockito;
import org.testng.annotations.DataProvider;
import org.testng.annotations.Test;
@@ -237,6 +238,49 @@ public class LeafStageToPinotQueryTest {
assertEquals(operand.getFunctionCall().getOperands().get(0).getFunctionCall().getOperator(),
"contains");
}
+ @Test
+ public void testMixedPredicateYieldsOnlyFilterKindOperatorsForPruners() {
+ // Shape of a reported failure: a scalar function canonicalizes to a
lowercase operator
+ // ("arraycontainsstring") and sits directly under AND, so pruners threw
+ // "No enum constant org.apache.pinot.sql.FilterKind.arraycontainsstring".
+ // Assert the two invariants pruners actually rely on, over the whole
normalized tree rather than a single
+ // wrapped node: every visited node is a FUNCTION (pruners dereference
getFunctionCall() unconditionally),
+ // and every operator resolves via FilterKind.valueOf.
+ // The AND mixes all the shapes that arrive non-FUNCTION or
non-FilterKind: a boolean scalar function, a
+ // bare boolean identifier, and a constant-folded FALSE literal.
+ Expression notEquals = makeCompound("NOT_EQUALS",
+ RequestUtils.getIdentifierExpression("label"),
RequestUtils.getLiteralExpression("Low"));
+ Expression scalarFn = makeCompound("arraycontainsstring",
+ RequestUtils.getIdentifierExpression("groups"),
RequestUtils.getLiteralExpression("y"));
+ Expression andExpr = makeCompound("AND", notEquals, scalarFn,
makeEquals("status", "open"),
+ RequestUtils.getIdentifierExpression("isActive"),
RequestUtils.getLiteralExpression(false));
+
+ Expression result =
LeafStageToPinotQuery.ensureFilterIsFunctionExpression(andExpr);
+
+ assertNotNull(result);
+ assertEquals(result.getFunctionCall().getOperator(), "AND");
+ assertEquals(result.getFunctionCall().getOperands().size(), 5, "No operand
should be dropped");
+ assertPrunerTraversableOperatorsAreFilterKinds(result);
+ }
+
+ /**
+ * Mirrors how segment pruners walk a filter: on each node they visit they
dereference the function call and
+ * resolve its operator via {@code FilterKind.valueOf}, then recurse only
into the operands of AND/OR/NOT.
+ * A non-FUNCTION node would NPE the pruners and an unknown operator would
throw, so both are asserted here.
+ */
+ private static void
assertPrunerTraversableOperatorsAreFilterKinds(Expression expression) {
+ // Pruners dereference getFunctionCall() unconditionally, so a
non-FUNCTION node here would NPE them.
+ Function function = expression.getFunctionCall();
+ assertNotNull(function, "Pruner-visited node must be a FUNCTION
expression, got: " + expression);
+ // Must not throw — this is exactly what segment pruners call on every
node they visit.
+ FilterKind filterKind = FilterKind.valueOf(function.getOperator());
+ if (filterKind == FilterKind.AND || filterKind == FilterKind.OR ||
filterKind == FilterKind.NOT) {
+ for (Expression operand : function.getOperands()) {
+ assertPrunerTraversableOperatorsAreFilterKinds(operand);
+ }
+ }
+ }
+
// --- AND/OR handling (shared logic, tested symmetrically) ---
@Test
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
