github-actions[bot] commented on code in PR #69344:
URL: https://github.com/apache/doris/pull/69344#discussion_r4488661491
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/RewriteSearchToSlots.java:
##########
@@ -150,13 +134,7 @@ private Expression rewriteSearch(Search search,
LogicalOlapScan scan) {
normalizedParentFieldName, subcolumnPath,
normalizedFieldName);
} else {
// Normal field - find slot directly
- Slot slot = findSlotByName(originalFieldName, scan);
- if (slot == null) {
- throw new AnalysisException(String.format(
- "Field '%s' not found in table for search: %s",
- originalFieldName, search.getDslString()));
- }
- checkInvertedIndexExists(scan.getTable(), slot.getName(),
search.getDslString(), false);
+ Slot slot = findSlotByName(originalFieldName, input);
Review Comment:
[P1] Include the final Variant path in the SEARCH cache identity
Binding to a visible alias leaves QsPlan unchanged when project pushdown and
VariantSubPathPruning replace its children. Consequently
`Filter(search('label:hello', child=v.city))` and the same plan with
`child=v.country` serialize identical DSL, binding names, descriptor IDs and
inherited index properties when each scan requests one path.
`FunctionSearch::dsl_cache_signature` hashes only those TSearchParam contents
plus max_expansions, under a shared segment prefix; it does not include the
actual path/iterator, and a cache hit returns before field resolution.
For indexed, fully materialized string paths and rows
`(1,{city:hello,country:world})`, `(2,{city:world,country:hello})`, run `SELECT
id FROM (SELECT id,v['city'] AS label FROM t) x WHERE search('label:hello')`,
then substitute `v['country'] AS label`. With inverted-index caching enabled
and no scoring, the correct results are 1 then 2, but the second query can
reuse the first bitmap and return 1. Include final physical column/path
identity in the cache key or execution metadata, and cover this with result-set
cache checks.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/jobs/executor/Analyzer.java:
##########
@@ -167,8 +167,8 @@ private static List<RewriteJob> buildAnalyzerJobs() {
// LogicalProject for normalize. This rule depends on
FillUpMissingSlots to fill up slots.
new NormalizeRepeat()
),
- // run CheckSearchUsage before CheckAnalysis to detect search() in
GROUP BY before it gets optimized
- bottomUp(new CheckSearchUsage()),
+ // Bind hidden DSL column dependencies before subquery expansion
and predicate pushdown.
+ topDown(new RewriteSearchToSlots()),
Review Comment:
[P2] Fold constant SEARCH arguments before parsing them
This pass now executes before expression normalization. For example, `SELECT
id FROM t WHERE search('hello', concat('{"default_field":', '"title"}'))` on an
indexed title column reaches `Search.getOptionsJson()` with a Concat child.
That method returns the expression's `toString()`, so
`SearchDslParser.parseOptions` receives `concat(...)` and throws `Invalid
search options JSON` instead of parsing the evaluated JSON string.
Previously expression normalization folded this constant before the late
binding pass. Computed constant DSL arguments such as `search(concat('title:',
'hello'))` have the same regression. Fold the DSL/options arguments before
parsing while retaining early dependency binding.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/SearchExpression.java:
##########
@@ -83,13 +82,8 @@ public boolean foldable() {
@Override
public SearchExpression withChildren(List<Expression> children) {
- // Validate that all children are SlotReference or ElementAt (for
variant subcolumns)
- for (Expression child : children) {
- if (!(child instanceof SlotReference || child instanceof
ElementAt)) {
- throw new IllegalArgumentException(
- "SearchExpression children must be SlotReference or
ElementAt instances");
- }
- }
+ // Rewrites such as null-rejection inference temporarily replace slots
with literals.
Review Comment:
[P2] Protect bound SEARCH slots from executable constant propagation
Consider `Project(id) -> Filter(title#1='hello' AND
SearchExpression(children=[title#1])) -> OlapScan(t)`, with an inverted index
on title. `ConstantPropagation.replaceAndConstants` infers `title#1 ->
'hello'`; its `canReplaceExpression` protects Match only, so this newly
permissive method installs the literal into SEARCH. SEARCH is nonfoldable, and
`CheckAfterRewrite` then raises `search() fields must reference OLAP scan
columns directly`.
Thus the ordinary valid query `SELECT id FROM t WHERE title='hello' AND
search('title:hello')` now fails planning; binding previously ran after
constant propagation. Shield bound SEARCH dependencies from executable constant
substitution as Match already does, while allowing temporary null-rejection
inference replacements.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/analysis/CheckAfterRewrite.java:
##########
@@ -214,6 +225,85 @@ private static boolean comparesVarBinary(Expression
conjunct) {
.stream().anyMatch(operand ->
operand.getDataType().isVarBinaryType()));
}
+ private void checkSearchIsUsedCorrectly(Plan plan) {
+ for (Expression expression : plan.getExpressions()) {
+ if (expression.anyMatch(expr -> expr instanceof Search)) {
+ throw new AnalysisException("search() could not be bound to an
OLAP table scan");
+ }
+ expression.foreach(expr -> {
+ if (!(expr instanceof SearchExpression)) {
+ return;
+ }
+ if (!(plan instanceof LogicalFilter) || !(plan.child(0)
instanceof LogicalOlapScan)) {
+ throw new AnalysisException("search() is only supported in
filters directly on an OLAP table scan");
+ }
+ LogicalOlapScan scan = (LogicalOlapScan) plan.child(0);
+ SearchExpression search = (SearchExpression) expr;
+ for (Expression child : search.children()) {
+ SlotReference slot;
+ if (child instanceof SlotReference) {
+ slot = (SlotReference) child;
+ } else if (child instanceof ElementAt && child.child(0)
instanceof SlotReference
+ && child.child(1) instanceof StringLiteral) {
+ slot = (SlotReference) child.child(0);
+ } else {
+ throw new AnalysisException("search() fields must
reference OLAP scan columns directly");
+ }
+ // VariantSubPathPruning replaces ElementAt with a slot
that retains its parent column.
+ checkInvertedIndexExists(scan.getTable(),
slot.getOriginalColumn().get().getName(),
+ search.getDslString(), child instanceof ElementAt
|| slot.hasSubColPath());
Review Comment:
[P2] Preserve Variant binding metadata for extracted aliases
This admits an extracted Variant slot, but its QsPlan still names the
visible alias. In the reduced tree `Filter(search('city:hello')) ->
Project(id,v['city'] AS city) -> Scan(t)`, pushdown produces a v.city child
while `ExprToThriftVisitor` sets `is_variant_subcolumn=false` because binding
`city` has no dot, and emits no parent/subpath metadata.
For `SELECT id FROM (SELECT id,v['city'] AS city FROM t) x WHERE
search('city:hello')`, a segment containing only v.country has a null v.city
iterator. `collect_slot_search_input` then throws `SEARCH index unavailable`
instead of calling `verify_missing_variant_path` and returning no matches as
direct `search('v.city:hello')` does. This fails even with caching disabled.
Derive Variant parent/path metadata from the final child while retaining the
logical alias key, and cover absent-path segments with result-set checks.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/jobs/executor/Analyzer.java:
##########
@@ -167,8 +167,8 @@ private static List<RewriteJob> buildAnalyzerJobs() {
// LogicalProject for normalize. This rule depends on
FillUpMissingSlots to fill up slots.
new NormalizeRepeat()
),
- // run CheckSearchUsage before CheckAnalysis to detect search() in
GROUP BY before it gets optimized
- bottomUp(new CheckSearchUsage()),
+ // Bind hidden DSL column dependencies before subquery expansion
and predicate pushdown.
+ topDown(new RewriteSearchToSlots()),
Review Comment:
[P2] Exclude SEARCH from cross-branch set-operation inference
With `Intersect(Project(title) -> Filter(search(title)) -> Scan(indexed_t),
Project(body) -> Scan(plain_t))`, `PullUpPredicates` remaps the left SEARCH
onto the set output. `InferPredicates.visitLogicalIntersect` then remaps it
onto body and adds a right-side filter. Its admission check only rejects
volatile/nonmovable expressions, so an unindexed body now fails the final index
check.
With spill disabled and matching string column types, `SELECT title FROM
indexed_t WHERE search('title:hello') INTERSECT SELECT body FROM plain_t` was
valid before early binding: scalar Search had no column inputs, so sibling
inference skipped it. EXCEPT has the same regression. Prevent these direct
sibling-branch substitutions from synthesizing index-dependent SEARCH
predicates; `InferPredicateByReplace`'s existing SEARCH exclusion does not
cover this route.
--
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]
