xiangfu0 commented on code in PR #18951:
URL: https://github.com/apache/pinot/pull/18951#discussion_r3557444416
##########
pinot-query-planner/src/test/java/org/apache/pinot/query/planner/logical/LeafStageToPinotQueryTest.java:
##########
@@ -237,6 +238,43 @@ public void testNotWithBooleanScalarFunctionWrapsOperand()
{
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 invariant pruners actually rely on: every operator they
traverse resolves via
+ // FilterKind.valueOf. This covers the whole normalized tree rather than a
single wrapped node.
+ 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"));
+
+ Expression result =
LeafStageToPinotQuery.ensureFilterIsFunctionExpression(andExpr);
+
+ assertNotNull(result);
+ assertPrunerTraversableOperatorsAreFilterKinds(result);
+ }
+
+ /**
+ * Mirrors how segment pruners walk a filter: they call {@code
FilterKind.valueOf(operator)} on each node they
+ * visit (which must not throw) and only recurse into the operands of
AND/OR/NOT.
+ */
+ private static void
assertPrunerTraversableOperatorsAreFilterKinds(Expression expression) {
+ Function function = expression.getFunctionCall();
+ if (function == null) {
+ return;
+ }
+ // 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) {
Review Comment:
Good catch — confirmed and fixed in 6acec67034.
All three pruners (`SinglePartitionColumnSegmentPruner`,
`MultiPartitionColumnsSegmentPruner`, `TimeSegmentPruner`) do:
```java
Function function = filterExpression.getFunctionCall();
FilterKind filterKind = FilterKind.valueOf(function.getOperator());
```
so a non-FUNCTION node NPEs them before the operator is ever resolved —
which is exactly what `ensureFilterIsFunctionExpression` exists to prevent. The
early `return` made the helper silently accept that regression.
Changes:
1. The helper now asserts the function call is non-null instead of
returning, so both invariants pruners depend on are covered: every visited node
is a FUNCTION, and every operator resolves via `FilterKind.valueOf`.
2. Broadened the predicate under test to mix all three shapes that arrive
non-FUNCTION or non-FilterKind — a boolean scalar function, a bare boolean
identifier, and a constant-folded `FALSE` literal — plus an assertion that no
operand is dropped.
I checked the assertion actually has teeth rather than assuming it:
reverting the identifier-wrapping in `ensureFilterIsFunctionExpression`
(`return expression;` instead of `return wrapAsEqualsTrue(expression);`) now
fails the test with
```
java.lang.AssertionError: Pruner-visited node must be a FUNCTION expression,
got: Expression(type:IDENTIFIER, identifier:Identifier(name:isActive))
```
Before this change, that same mutation passed.
--
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]
