Repository navigation
perf: optimize some multi-value string operators to allow planning to specialized ListFilteredVirtualColumn without extractionFn - #20469
Conversation
… specialized ListFilteredVirtualColumn without extractionFn
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| @Override | ||
| public String getQuery() | ||
| { | ||
| return QUERIES.get(Integer.parseInt(query)).rhs; |
| @Override | ||
| public List<String> getDatasources() | ||
| { | ||
| return Collections.singletonList(QUERIES.get(Integer.parseInt(query)).lhs); |
FrankChen021
left a comment
FrankChen021
left a comment
There was a problem hiding this comment.
🟡 Changes recommended
Reviewed all 9 changed files in the complete merge-base diff, including the benchmark additions, selector and mapping changes, SQL expression conversion, and associated tests.
The PR should not merge until the removed public selector-helper overloads are retained for source and binary compatibility and nondeterministic one-input delegates are excluded from deferred dictionary evaluation. The optimized deterministic paths and duplicate-value mapping were otherwise traced through the virtual-column registry and selector implementations.
Validation: static review only; git diff --check f45885a53b1a16d6930caa30b4077d05e25a5a86 b4adf42c27d2667c1453add04e7e39abf219288b passed. No builds or tests were run per review instructions.
| Severity | Findings |
|---|---|
| P0 | 0 |
| P1 | 1 |
| P2 | 1 |
| P3 | 0 |
| Total | 2 |
This is an automated review by Codex GPT-5.6-Luna(max)
After addressing the findings or replying to the comments, you can request another review from me to trigger a new automated review.
| return null; | ||
| } | ||
| final Expr.BindingAnalysis bindingAnalysis = plannerContext.parseExpression(arg.getExpression()).analyzeInputs(); | ||
| if (bindingAnalysis.getRequiredBindings().size() != 1 |
There was a problem hiding this comment.
[P2] Do not defer nondeterministic delegates by dictionary
Finding: The new eligibility check accepts any one-binding scalar string expression without checking BindingAnalysis.isNonDeterministic(). A string-producing one-column expression that includes NOW() can therefore be lifted into an ExpressionVirtualColumn; its single-input plan uses SingleStringInputDeferredEvaluationExpressionDimensionSelector and evaluates the expression once per dictionary value instead of per row, changing filtered or grouped results for nondeterministic expressions.
Suggestion: Reject nondeterministic binding analyses here (or force the plain expression path) and add a regression test covering a one-column nondeterministic delegate.
There was a problem hiding this comment.
i guess any expression selector that doesn’t run the expression every row has that problem with such an expression; that flag is pretty new and only used by now() which is only a native expression so it currently cannot happen here, and I don't think this is the appropriate place to fix it. Like, i think the expression planner layer should handle it and not allow it to use any of the special path selectors like deferred dictionary encoded evaluation on lookup or caching selectors or similar.
I will look into fixing this problem at that layer in a follow-up PR.
There was a problem hiding this comment.
I'm not sure this is really a problem. We shouldn't guarantee that non-deterministic expressions necessarily run on every row, it ties our hands too much in situations like this. I believe it is fine to have the policy that non-deterministic expressions will run one or more times during the query, without promises beyond that.
There was a problem hiding this comment.
Understood. Given the intended policy that nondeterministic expressions may run one or more times during a query, the dictionary-deferred evaluation here is acceptable and I withdraw this finding. I rechecked the current delegate and selector path and found no separate actionable issue in this thread.
gianm
left a comment
gianm
left a comment
There was a problem hiding this comment.
A little birdie told me that the following queries would involve regressions or behavior changes on this branch. Please look into them.
SELECT COUNT(*) FROM druid.foo WHERE MV_FILTER_ONLY(dim3, ARRAY['zzz']) IS NULL
SELECT COUNT(*) FROM druid.foo WHERE MV_FILTER_ONLY(dim3, ARRAY['zzz']) >= 'a'
SELECT COUNT(*) FROM druid.foo f INNER JOIN lookup.lookyloo l ON f.dim2 = l.k WHERE MV_FILTER_ONLY(UPPER(l.v), ARRAY['XA']) = 'XA'
SELECT f.dim1, l.v, l2.v FROM druid.foo f INNER JOIN lookup.lookyloo l ON MV_FILTER_ONLY(LOWER(f.dim3), ARRAY['a']) = l.k INNER JOIN lookup.lookyloo l2 ON f.dim2 = l2.k
SELECT f.dim1, l.v, l2.v FROM druid.foo f INNER JOIN lookup.lookyloo l ON f.dim2 = l.k INNER JOIN lookup.lookyloo l2 ON MV_FILTER_ONLY(SUBSTRING(l.v, 2), ARRAY['a']) = l2.k
SELECT MV_FILTER_REGEX(LOWER(dim2), '^a'), COUNT(*) FROM druid.foo GROUP BY 1
SELECT MV_FILTER_ONLY(NVL(dim3, 'x'), ARRAY['x']), COUNT(*) FROM druid.foo GROUP BY 1
SELECT MV_FILTER_ONLY(NVL(dim3, 'x'), ARRAY['x', 'a']), COUNT(*) FROM (SELECT dim3 FROM druid.foo LIMIT 10) GROUP BY 1
SELECT COALESCE(MV_FILTER_ONLY(UPPER(dim3), ARRAY['B']), 'none'), COUNT(*) FROM druid.foo GROUP BY 1
SELECT MV_FILTER_NONE(LOOKUP(dim3, 'lookyloo'), ARRAY['xa']), COUNT(*) FROM druid.foo GROUP BY 1 ORDER BY 2 DESC LIMIT 2
4 participants
Description
Optimizes SQL planning so that multi-value string operators that would plan to a specialized
ListFilteredVirtualColumnweresqlUseExtractionFnset to true, but currently plan to a pure expression, to instead still plan to the specialized column pointed at an expression virtual column. If the expression virtual column is actually a transform on top of a dictionary encoded column, the performance can be approximately the same as the extractionFn version since those selectors can do deferred evaluation and provide access to the underlying dictionary ids.From the added benchmark:
basicdataset: 1.5M rows,dimMultivalEnumeratedhas 5 distinct valuesGROUP BY MV_FILTER_ONLY(LOOKUP(dimMultivalEnumerated, ...), ARRAY['greeting'])GROUP BY MV_FILTER_NONE(LOOKUP(dimMultivalEnumerated, ...), ARRAY['greeting'])GROUP BY MV_FILTER_REGEX(LOOKUP(dimMultivalEnumerated, ...), '^g.*')GROUP BY MV_FILTER_PREFIX(LOOKUP(dimMultivalEnumerated, ...), 'g')WHERE MV_FILTER_ONLY(LOOKUP(dimMultivalEnumerated, ...), ARRAY['greeting']) = 'greeting'GROUP BY MV_FILTER_ONLY(SUBSTRING(dimMultivalEnumerated, 1, 1), ARRAY['B'])GROUP BY MV_FILTER_ONLY(REGEXP_EXTRACT(dimMultivalEnumerated, '^[A-Z][a-z]'), ARRAY['Ba'])grouperdataset: 2 segments × 1.5M rows, multi-value string columns with 1M distinct values; the lookup maps 1M keys to 1000 valuesGROUP BY MV_FILTER_ONLY(LOOKUP("multi-string-Uniform-1_000_000", ...), ARRAY['bucket-1', 'bucket-2'])GROUP BY MV_FILTER_ONLY(LOOKUP("multi-string-ZipF-1_000_000", ...), ARRAY['bucket-1', 'bucket-2'])GROUP BY MV_FILTER_PREFIX(LOOKUP("multi-string-Uniform-1_000_000", ...), 'bucket-1')WHERE MV_FILTER_ONLY(LOOKUP("multi-string-Uniform-1_000_000", ...), ARRAY['bucket-1']) = 'bucket-1'GROUP BY MV_FILTER_ONLY(SUBSTRING("multi-string-Uniform-1_000_000", 1, 2), ARRAY['12', '34'])GROUP BY MV_FILTER_ONLY(REGEXP_EXTRACT("multi-string-Uniform-1_000_000", '[0-9]{2}$'), ARRAY['12', '34'])