You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "xiangfu0 (via GitHub)" <gi...@apache.org> on 2026/07/09 23:30:06 UTC
[PR] Fix MSE routing filter for boolean scalar function predicates [pinot]
xiangfu0 opened a new pull request, #18951:
URL: https://github.com/apache/pinot/pull/18951
## Problem
With `usePhysicalOptimizer=true`, a query whose `WHERE` clause contains a boolean scalar function such as `arrayContainsString` fails at plan time:
```
Error composing query plan: No enum constant org.apache.pinot.sql.FilterKind.arraycontainsstring
```
Repro (fails):
```sql
SET useMultistageEngine=true;
SET usePhysicalOptimizer=true;
SELECT /*+ joinOptions(is_colocated_by_join_keys='true') */ f.PrimaryKey
FROM finding f JOIN asset a ON f.ColocationKey = a.ColocationKey
WHERE arrayContainsString(a.AssetInfo_Groups, 'y') AND f.CID = 'x' AND a.CID = 'x'
LIMIT 100
```
The equivalent single-stage / non-physical-optimizer query works:
```sql
SELECT * FROM asset WHERE arrayContainsString(AssetInfo_Groups, 'y') AND CID = 'x' LIMIT 100
```
## Root cause
With the V2 physical optimizer, `LeafStageWorkerAssignmentRule` calls `LeafStageToPinotQuery.createPinotQueryForRouting(...)` to build a `PinotQuery` **used only for broker-side segment pruning / routing**. That filter is built via `CalciteRexExpressionParser`, which does **not** run `PredicateComparisonRewriter`.
As a result, `arrayContainsString(col, 'y')` is emitted as a filter operand whose operator is the lowercased canonical function name `arraycontainsstring`, sitting directly under `AND`. Segment pruners then call `FilterKind.valueOf("arraycontainsstring")`, which throws.
Two reasons the bug is narrow:
- The **single-stage path works** because `PredicateComparisonRewriter` wraps non-`FilterKind` boolean functions into the canonical `EQUALS(func, TRUE)` form, which pruners understand.
- Comparisons like `!=` never failed, because they are emitted as `SqlKind` names (`NOT_EQUALS`, `GREATER_THAN`, ...) that happen to be valid `FilterKind` constants.
## Fix
`LeafStageToPinotQuery.ensureFilterIsFunctionExpression` is the existing hook that normalizes the routing filter "so segment pruners can process it" (it already wraps bare boolean identifiers and constant-folded literals). This PR extends it: any `FUNCTION` whose operator is not a valid `FilterKind` (and is not `AND`/`OR`/`NOT`) is wrapped as `EQUALS(func, TRUE)` — the same canonical form the single-stage `PredicateComparisonRewriter` produces.
This is safe and conservative for pruning. All three pruners treat the wrapped node as non-prunable and keep all segments:
- `SinglePartitionColumnSegmentPruner` / `MultiPartitionColumnsSegmentPruner`: `EQUALS` with a non-identifier operand 0 → `return true` (segment kept).
- `TimeSegmentPruner`: `EQUALS` whose operand 0 is not the time column → `return null` (no interval constraint).
So no segment that could match is ever pruned away.
Note this filter is broker-local (routing only) and is never serialized to servers, so there is no wire-format or mixed-version impact.
The PR also collapses the repeated `EQUALS(x, TRUE)` construction in that method into a small `wrapAsEqualsTrue` helper.
## Testing
`LeafStageToPinotQueryTest` (29 tests, all passing), including new cases that fail without the fix:
- `testNonFilterKindFunctionWrappedAsEqualsTrue`
- `testAndWithNonFilterKindFunctionWrapsFunction` / `testOrWithNonFilterKindFunctionWrapsFunction`
- `testNotWithNonFilterKindFunctionWrapsChild`
- `testNonEqualsFilterKindFunctionPassedThroughUnchanged` — guards against over-wrapping valid `FilterKind`s (e.g. `IN`)
- `testReportedQueryShapeProducesOnlyFilterKindOperators` — mirrors the reported query's `WHERE` shape and asserts the invariant pruners rely on: every operator a pruner traverses is a valid `FilterKind`, so `FilterKind.valueOf` never throws
`spotless:check`, `checkstyle:check`, and `license:check` pass clean on `pinot-query-planner`.
## Follow-ups (not in this PR)
- Consider unifying this routing-filter normalization with `PredicateComparisonRewriter` (or extracting the shared wrapping helper) so the two cannot silently diverge. A naive "just call PCR here" is risky: PCR performs transforms undesirable for a best-effort routing filter (VECTOR_SIMILARITY operand validation, a `default` branch requiring literal operands, comparison flipping), while `ensureFilterIsFunctionExpression` does routing-only normalization PCR does not (drop `LITERAL true`, `LITERAL false` → always-false, unwrap single-operand `AND`/`OR`).
- Add a cluster-level integration test with a partitioned table + multi-value STRING column exercising `arrayContainsString` under `usePhysicalOptimizer=true`, to cover the full routing pipeline end to end.
## Labels
`bugfix`, `multi-stage`
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Follow-up to #18941: routing-filter pruner-invariant test + construction cleanup [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 merged PR #18951:
URL: https://github.com/apache/pinot/pull/18951
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Follow-up to #18941: routing-filter pruner-invariant test + construction cleanup [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Follow-up to #18941: routing-filter pruner-invariant test + construction cleanup [pinot]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Follow-up to #18941: routing-filter pruner-invariant test + construction cleanup [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #18951:
URL: https://github.com/apache/pinot/pull/18951#issuecomment-4930865506
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/18951?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 56.96%. Comparing base ([`849c620`](https://app.codecov.io/gh/apache/pinot/commit/849c6206237fd7ce7151229db87638bac9ff54c0?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`13bf946`](https://app.codecov.io/gh/apache/pinot/commit/13bf946d81e6f268cfefc9bbfaad1ba60562413e?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
> :exclamation: There is a different number of reports uploaded between BASE (849c620) and HEAD (13bf946). Click for more details.
>
> <details><summary>HEAD has 16 uploads less than BASE</summary>
>
>| Flag | BASE (849c620) | HEAD (13bf946) |
>|------|------|------|
>|java-21|5|1|
>|unittests|2|1|
>|temurin|5|1|
>|unittests2|1|0|
>|integration|3|0|
>|integration1|1|0|
>|custom-integration1|1|0|
>|integration2|1|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #18951 +/- ##
============================================
- Coverage 64.92% 56.96% -7.96%
+ Complexity 1347 1 -1346
============================================
Files 3395 2605 -790
Lines 212564 153734 -58830
Branches 33518 25077 -8441
============================================
- Hits 137998 87574 -50424
+ Misses 63424 58697 -4727
+ Partials 11142 7463 -3679
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/18951/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [custom-integration1](https://app.codecov.io/gh/apache/pinot/pull/18951/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration](https://app.codecov.io/gh/apache/pinot/pull/18951/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration1](https://app.codecov.io/gh/apache/pinot/pull/18951/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration2](https://app.codecov.io/gh/apache/pinot/pull/18951/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-21](https://app.codecov.io/gh/apache/pinot/pull/18951/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.96% <100.00%> (-7.96%)` | :arrow_down: |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/18951/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.96% <100.00%> (-7.96%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/18951/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.96% <100.00%> (-7.96%)` | :arrow_down: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/18951/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.96% <100.00%> (+0.01%)` | :arrow_up: |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/18951/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/pinot/pull/18951?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org