Copilot commented on code in PR #18951:
URL: https://github.com/apache/pinot/pull/18951#discussion_r3556707735
##########
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:
`assertPrunerTraversableOperatorsAreFilterKinds()` returns early when
`expression.getFunctionCall()` is null, but real segment pruners
unconditionally dereference `getFunctionCall()` and would NPE. This makes the
new invariant test weaker than intended because a regression that yields a
non-FUNCTION node could still pass. Consider asserting the function call is
non-null instead of returning.
--
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]
