Repository navigation
Rewrite arrayContains on multi-value columns into an indexable EQUALS filter - #20000
raghavyadav01 wants to merge 3 commits into
Conversation
A boolean arrayContainsInt/arrayContainsString(mvCol, literal) predicate was evaluated as a scalar expression on every document, while the equivalent mvCol = literal can use the dictionary, inverted index and segment pruning. Add ArrayContainsFilterOptimizer to rewrite it when the first argument is a multi-value column whose type matches the function and the second is a literal. Negations are left untouched since != on an MV column has different semantics.
…ewriting The EQUALS rewrite passed the raw literal through, but the scalar function converts it first (truncating doubles, casting longs, trimming strings) and NumericalFilterOptimizer does not convert literals for multi-value columns, so e.g. arrayContainsInt(mvCol, 5.0) failed after the rewrite. Apply the same PinotDataType conversion as the scalar path and skip the rewrite when it fails. Also document the expression override interaction and add combined-predicate tests.
xiangfu0
left a comment
There was a problem hiding this comment.
Nice, well-scoped optimizer. I walked through the semantics and they hold up: on an MV column = matches when any element equals the literal (BaseRawValueBasedPredicateEvaluator#applyMV), which is exactly arrayContains*, and the negation refusal is right because MV != means "all elements differ" rather than "no element matches". The convertLiteral switch mirrors the scalar path (ScalarTransformFunctionWrapper / FunctionInvoker.convertTypes) so the rewritten EQUALS literal lands on the same value, including the 5.0 -> 5, ' 5' -> 5, 5 -> '5' cases, and it falls back to the scalar function on any conversion failure. Schema-null, SV columns, type mismatch, missing column, and non-identifier/non-literal args are all handled, and the unit + integration tests cover the important cases (index usage, scalar equivalence, NOT complement). I confirmed the expression-override ordering noted in the Javadoc. Only minor comments inline; approving.
| /// multi-value column means "some value does not match", which differs from "no value matches" | ||
| /// | ||
| /// NOTE: The rewrite runs before broker-side expression overrides are applied, so an expression override keyed on an | ||
| /// `arrayContains*` predicate that gets rewritten no longer matches. |
There was a problem hiding this comment.
I verified this ordering claim against the broker path: the primary _queryOptimizer.optimize(...) in compileRequest runs before handleExpressionOverride(...) in the routing step, so an override keyed on an arrayContains* expression really would stop matching once we rewrite it here. It's a genuine, if niche, behavior change — good that it's documented. Might be worth a one-liner in the PR description / release notes too so it isn't a surprise for anyone relying on such an override.
| value = parameterType.convert(literalContext.getDoubleValue(), PinotDataType.DOUBLE); | ||
| break; | ||
| case BIG_DECIMAL: | ||
| value = parameterType == PinotDataType.STRING ? literalContext.getStringValue() |
There was a problem hiding this comment.
Minor: this BIG_DECIMAL special-case for the STRING target is effectively a no-op — getStringValue() already returns BIG_DECIMAL.toString(value) (toPlainString()), which is exactly what STRING.convert(bigDecimal, BIG_DECIMAL) would produce. You could fold it into the general parameterType.convert(...) path to drop the branch, though it's fine to leave as-is for readability.
| default: | ||
| return null; | ||
| } | ||
| } catch (Exception e) { |
There was a problem hiding this comment.
Catching Exception here is reasonable since any conversion failure should just keep the scalar function, but it also swallows truly unexpected errors silently. A one-line comment noting the intent (fall back to the scalar path on any non-convertible literal) would make that explicit for future readers. Not blocking.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #20000 +/- ##
============================================
- Coverage 69.67% 69.66% -0.01%
+ Complexity 1491 1485 -6
============================================
Files 3548 3549 +1
Lines 234929 235089 +160
Branches 37668 37710 +42
============================================
+ Hits 163677 163786 +109
- Misses 58582 58622 +40
- Partials 12670 12681 +11
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Problem
On a multi-value column,
arrayContainsString(col, 'v')andarrayContainsInt(col, 5)are planned as a scalar-function expression filter on every segment. The equivalentcol = 'v'(an MV=matches when any element matches) uses the inverted index and lets segments prune to empty through the dictionary, bloom filter or min/max value. On a table where the value is rare, the difference is large: the=form prunes almost every segment and scans nothing, whilearrayContainsevaluates the function on every document of every segment.Change
New
ArrayContainsFilterOptimizer, run right afterFlattenAndOrFilterOptimizer, rewritesarrayContainsInt/String(col, literal)(bare or= true) intocol = literalwhen:colis a plain identifier of an MV column whose type matches the function (INT forarrayContainsInt, STRING forarrayContainsString);PinotDataTypeconversion the scalar path uses (5.0→5,' 5'→5,5→'5'), so the rewritten filter returns the same rows. If conversion fails (e.g.arrayContainsInt(col, 'foo')), the function is kept.It only walks AND/OR. It never rewrites under
NOT,= falseor<> true, because!=on an MV column means "no element equals", not the negation of "contains".The output is a normal EQUALS, so
MergeEqInFilterOptimizercan merge an OR of them into IN (any element in the set, same semantics). The multi-stage leaf stage runs the sameQueryOptimizer, so it gets the rewrite as well.Note: a table-level expression override keyed on the
arrayContainsexpression would no longer match after the rewrite (documented in the class Javadoc).Tests
QueryOptimizerTest.testArrayContainsFilter: rewrites (bare,= true, snake_case, inside AND, merged into IN), literal conversion cases, and every shape that must stay unchanged (negations, column or function argument, missing column, SV column, type mismatch, unconvertible literal).ArrayContainsMultiValueQueriesTest: on an MV INT column with an inverted index, the rewritten query returns the same count as the scalar evaluation and ascol = 225, withnumEntriesScannedInFilter= 0 (400,000 on master); also literal conversion, a missing value,NOT arrayContains, OR, same-column AND and nested AND/OR/NOT, each compared with the scalar result.*Optimizer*,ArrayContainsMultiValueQueriesTest,InnerSegment*MultiValue*andInterSegment*MultiValue*pass (117 tests).🤖 Generated with Claude Code