Repository navigation
[fix](search) Bind SEARCH fields before predicate pushdown - #69344
feiniaofeiafei wants to merge 6 commits into
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
feiniaofeiafei
changed the title
|
run buildall |
TPC-H: Total hot run time: 28638 ms |
TPC-DS: Total hot run time: 150587 ms |
ClickBench: Total hot run time: 24.5 s |
FE Regression Coverage ReportIncrement line coverage |
FE UT Coverage ReportIncrement line coverage |
073b20b to
83ed119
Compare
|
run buildall |
TPC-H: Total hot run time: 28486 ms |
TPC-DS: Total hot run time: 150810 ms |
ClickBench: Total hot run time: 24.24 s |
FE Regression Coverage ReportIncrement line coverage |
### Background SEARCH fields are bound during analysis before score binding runs in rewrite. Bound SEARCH predicates use SearchExpression instead of the scalar Search function. ### What problem does this PR solve? Issue Number: None Related PR: apache#69344 (early SEARCH binding), apache#69292 (score binding) PR Type: Bugfix Problem Summary: Valid SEARCH queries that call score() are rejected because score binding only recognizes the unbound Search type. Recognize SearchExpression in the WHERE predicate check, scoring alias propagation and HAVING/QUALIFY validation. ### Details - **Frequency:** SEARCH scoring queries after early SEARCH binding. - **Root cause:** The downstream score binder checks the expression type from the previous rule ordering. - **Why existing tests missed it:** The targeted SEARCH suites did not cover score(); existing score regression suites expose the failure. - **Fix:** Replace all three Search type checks with SearchExpression and update the import. - **Other paths and branches:** MATCH recognition and score usage restrictions remain unchanged. - **Behavior change:** Valid SEARCH scoring queries can pass score binding. ### Testing FE Checkstyle and git diff --check passed. Built FE with DISABLE_BUILD_UI=ON ./build.sh --fe and deployed it to the development cluster. The five targeted FE test classes executed 147 tests with zero failures, errors or skips, including 82 BindScoreToOlapScanTest cases. Ran 21 targeted regression suites: 20 passed, one failed because the existing BE lacks the keyword MATCH scoring change from apache#69790. Its actual 0.290716 score matches the BE-version expected output, while the current expected output is 0.983863. Re-running that suite against the existing generated expected output from the running BE commit passed. Repository expected outputs were not changed. The previous SEARCH score binding error was not reproduced. The initial local FE unit-test attempt stopped during dependency resolution due to Maven-cache sandbox permissions; remote tests provide the validation above. ### Release note Fix planning failures when using score() with SEARCH predicates. ### Check List (For Author) - Behavior changed: Yes (see Details) - Does this need documentation: No - Pre-submission status: Partially verified: current keyword MATCH expected scores require a BE containing apache#69790; validation used the existing BE ### AI Disclosure - Tool: OpenAI Codex - Generated: Code and commit description - Human review: Pending
83ed119 to
00fbcd2
Compare
|
run buildall |
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Request changes: five independently verified findings (one P1, four P2) affect SEARCH result correctness and valid-query admission at head 00fbcd2fd54cf021c50c704cbb866add9eace6dd. Each finding is inline. No existing inline findings were present to deduplicate or retain as blockers.
The intended CTE/NOT IN fix is supported by the early dependency-binding path and the added FE plan checks. The remaining regressions are: alias Variant-path bitmap-cache collisions, executable constant propagation of SEARCH children, parsing constant SQL arguments before folding, missing Variant metadata for extracted aliases, and INTERSECT/EXCEPT inference copying SEARCH into an unindexed sibling.
Critical checkpoint conclusions:
| Checkpoint | Conclusion |
|---|---|
| Goal and proof | Early binding exposes SEARCH dependencies before Apply/pushdown. Added tests cover the reported plan, but the five findings prevent correctness approval; EXPLAIN does not prove returned rows. |
| Focus, clarity and reuse | Focused SEARCH changes reuse existing pushdown and consolidate validation; no unrelated production edits found. |
| Concurrency and locks | Synchronous query-local planner state; no new threads, shared mutable state, heavy locked operation or lock-order change. |
| Lifecycle and initialization | No new resource ownership, release path or static initialization dependency. Retained QsPlan versus rewritten children causes the two alias metadata findings. |
| Configuration | No added or changed configuration. Existing inverted-index caching enables the P1 case. |
| Compatibility and rolling upgrades | No binary/storage-format change. SQL compatibility regresses in the constant-expression, constant-propagation and set-operation cases. |
| Thrift/protobuf IDs | No gensrc or field-ID changes. Existing SEARCH serialization omits physical path identity and misclassifies extracted aliases. |
| Parallel paths | Inspected CTE producers/consumers, joins including mark/semi/anti/outer paths, projections, UNION/INTERSECT/EXCEPT, score and Variant paths. The inline findings identify the concrete gaps. |
| Special conditions and errors | Early position/empty-DSL checks and final placement/index checks are intentional. Eliminated predicates need no index; several valid surviving predicates are incorrectly rejected as described inline. |
| Test coverage | Inspected all three changed FE test files and six changed regression suites, including negative cases. Constant SQL arguments, executable constant substitution, set-operation sibling inference and actual alias cache/absent-path results need coverage. |
| Test results | No result files changed. New suites primarily assert EXPLAIN structure; sparse-Variant result checks are explicitly deferred. No builds, tests or runtime reproduction were executed during this static review. Author-reported runs were not treated as reviewer execution. |
| Observability | Existing analysis errors identify SEARCH/field/index context. No new distributed operation requiring additional metrics found. |
| Transactions and persistence | No transaction, EditLog, replay or failover path changed. CREATE VIEW retains rewritten original SQL; no new options-persistence issue substantiated. |
| Writes and crash atomicity | No data-write or storage-lifecycle change. The P1 issue affects query bitmap reuse and returned rows. |
| FE/BE variables and send paths | No new variables. Existing child translation and binding serialization were traced into BE iterator selection, missing-path handling and cache lookup. |
| Performance and memory | Per-field binding/index checks remain bounded by query/table metadata. No separate material CPU, allocation or memory-ownership defect substantiated. |
| Other/core invariants | Visibility versions and delete bitmaps are untouched. Unsupported paths fail explicitly; the inline findings identify wrong-result or valid-query failures. |
User focus: no additional focus was supplied; the complete 18-file PR was reviewed. All initial risk items and candidates have explicit dispositions in the shared ledger. Static inspection provides the evidence; execution behavior was not tested.
Convergence: converged after 3 cumulative rounds. All three reviewers returned NO_NEW_VALUABLE_FINDINGS in the final round, required static coverage is complete, and no candidate or risk question remains unresolved. The findings still require changes; convergence describes review coverage, not approval.
Review converged after 3 round(s).
| originalFieldName, search.getDslString())); | ||
| } | ||
| checkInvertedIndexExists(scan.getTable(), slot.getName(), search.getDslString(), false); | ||
| Slot slot = findSlotByName(originalFieldName, input); |
There was a problem hiding this 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.
| "SearchExpression children must be SlotReference or ElementAt instances"); | ||
| } | ||
| } | ||
| // Rewrites such as null-rejection inference temporarily replace slots with literals. |
There was a problem hiding this 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.
| // 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()), |
There was a problem hiding this 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.
| } | ||
| // 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()); |
There was a problem hiding this 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.
| // 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()), |
There was a problem hiding this 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.
TPC-H: Total hot run time: 28703 ms |
TPC-DS: Total hot run time: 154227 ms |
ClickBench: Total hot run time: 24.08 s |
### Background SEARCH binds its hidden DSL fields before predicate pushdown. The resulting dependencies can be renamed, propagated, and replaced by extracted Variant scan slots before BE execution. ### What problem does this PR solve? Issue Number: None Related PR: apache#69344 (review follow-up) PR Type: Bugfix Problem Summary: Fix all five review findings: aliased Variant paths could share cached bitmaps, executable constant propagation could replace SEARCH fields with literals, computed DSL/options were parsed before folding, extracted aliases lost missing-path metadata, and set-operation inference could copy SEARCH into an unindexed sibling. Preserve physical bindings and source-index dependencies while retaining early binding. ### Details - **Change map:** 111 changed production lines, 215 changed test-source lines, and 49 generated result lines. - **Frequency:** Triggered by the described SQL shapes; cache wrong results require bitmap cache reuse without scoring. - **Root cause:** The earlier binding stage exposed SEARCH to normalization and inference while serialization and cache identity still relied on logical DSL names. - **Why existing tests missed it:** Plan-only alias and CTE checks did not validate cache reuse or missing-path segment results; constant arguments and direct set-operation sibling inference lacked coverage. - **Fix:** Fold SEARCH arguments before parsing; protect executable SEARCH dependencies from constant substitution and inferred cloning; derive Variant metadata from final scan descriptors or element_at children; encode sorted logical-to-storage bindings in cache signatures using length prefixes. - **Other paths and branches:** Cover materialized and retained element_at Variant paths, INTERSECT/EXCEPT and the corresponding join inference gate. Preserve temporary inference replacements and the scoring cache bypass. No protocol IDs, storage formats, or configurations change. ### Testing - Built FE and ASAN BE with build.sh and deployed the artifacts on the development cluster. - 216 FE tests passed across nine targeted classes. - FunctionSearchTest and FunctionSearchNullTest passed using run-be-ut.sh. - 19 related regression suites passed; generated the new result file using run-regression-test.sh and reran it for comparison. - The original 24-query cross-connection cache reproduction produced zero mismatches after the fix. - FE Checkstyle, clang-format 16, clang-tidy, and BE build-hygiene checks passed. ### Release note Fix wrong SEARCH results when cached searches use the same alias for different Variant paths. Allow valid SEARCH queries with constant expressions, equalities, missing Variant paths through aliases, and INTERSECT/EXCEPT against unindexed sibling columns. ### Check List (For Author) - Behavior changed: Yes (see Details). - Does this need documentation: No. - Pre-submission status: Ready. ### AI Disclosure - Tool: Codex. - Generated: Code, tests, and this description. - Human review: Pending author review.
|
/review |
There was a problem hiding this comment.
Static review of head 7ef8f50: one new P2 finding, reported inline. Aliased Variant roots are admitted for NESTED SEARCH, but logical nested ownership and final physical leaf metadata disagree, causing execution failure in NestedGroup-capable builds.
All five existing reports are addressed for their stated scenarios: 4488661491 (physical cache identity), 4488661665 (constant propagation), 4488662116 (computed arguments), 4488662248 (ordinary alias missing-path metadata), and 4488662377 (INTERSECT/EXCEPT inference). No previously reported P0/P1 remains independently confirmed, so none is reposted or carried as blocking. The new NESTED-owner issue is distinct from ordinary leaf missing-path metadata.
Required checkpoint conclusions:
- Goal and evidence: early binding exposes dependencies before subquery expansion and supports the intended CTE/NOT IN, alias, join and UNION plans. Tests cover those planning paths and selected result sets; NESTED alias execution remains incomplete as reported inline.
- Scope and reuse: changes are focused on SEARCH binding/validation, dependent optimizer rules, physical metadata/cache identity and tests; existing folding, slot and index APIs are reused.
- Concurrency and locks: no new worker, lock or shared mutable state; query-owned FE structures and existing BE cache handles remain in use.
- Lifecycle and static initialization: no new retained resource, ownership cycle or cross-translation-unit initializer; signature buffers are local and scan descriptors keep their existing lifetime.
- Configuration: no configuration item added; tests use existing settings.
- Compatibility: no persistent storage format change or new wire field. Additional cache-key material is process-local. Artifact ABI and mixed-version execution were not run.
- Thrift/protobuf IDs: no gensrc definitions changed; cross-branch ID comparison is not applicable.
- Parallel paths: traced project/CTE/set mappings, extracted and retained ElementAt Variant children, ordinary missing-path handling, index-dependent inference and scoring. NESTED owner mapping is the identified gap.
- Conditions and errors: new inference/constant-substitution guards and final shape/index checks have concrete failure scenarios; errors propagate through existing exception/Status boundaries. No other guard defect substantiated.
- Test coverage: inspected all changed FE/BE tests and regression suites. New tests cover cache order/path/swap separation, absent segments, computed arguments, equality and set operations. Aliased NESTED execution lacks coverage.
- Expected results: all 16 new result blocks agree with inserted rows: city=1, country=2, equality/computed arguments=1, INTERSECT/EXCEPT=hello, absent-city queries=1. Output generation and author-reported passing runs were not independently executed.
- Observability: field-specific analysis errors and existing BE binding diagnostics remain; no additional metric requirement identified for this metadata change.
- Persistence and failover: no journal/replay or persistent metadata change.
- Writes and atomicity: no production data-write or transaction path change; committed visibility, visible-version filtering and delete bitmap rules are unchanged.
- FE-to-BE passing: final physical leaf metadata uses existing SEARCH bindings through the shared translator; the separate NESTED root still needs consistent resolution, as reported.
- Performance and memory: new cache identity work is O(F log F) for F query fields, with local metadata copies and no new row-wise processing or retained data-sized allocation. No benchmarks or memory measurements were run.
- Other concerns and focus: no additional user focus was supplied. All 29 changed paths and both initial risk focuses were reviewed; the dotted-key suspicion was dismissed against the existing writer/reader canonicalization contract.
Review converged in 2 rounds after complete coverage, candidate disposition, and a final sweep. Validation scope is static inspection only: no builds, tests, benchmarks, service startup, source edits or submodule initialization were performed. Complex CTE and several alias/join/UNION regressions are EXPLAIN-only; optional NestedGroup execution and runtime results were not exercised here.
Review converged after 2 round(s).
| List<String> subPath = descriptor.getSubColPath(); | ||
| if (subPath != null && !subPath.isEmpty()) { | ||
| thriftBinding.setIsVariantSubcolumn(true); | ||
| thriftBinding.setParentFieldName(descriptor.getColumn().getName()); |
There was a problem hiding this comment.
[P2] Bind NESTED owners through the final physical field mapping
In a build with NestedGroup search enabled and an indexed v.items.msg, this newly admitted plan fails execution:
Filter(search('NESTED(doc.items, msg:hello)'))
Project(id, v AS doc)
Scan(t)
After pushdown the leaf is v.items.msg, so this emits parent_field_name=v, while convertQsNodeToThrift retains nested_path=doc.items. bind_nested_column in vsearch.cpp compares those roots (v != doc) and leaves nested_column unset. VariantNestedSearchEvaluator::evaluate then returns Column 'doc' not bound to current read schema for nested query, although the direct NESTED(v.items, ...) form can execute. Resolve the NESTED owner and full array path through the final physical binding while retaining logical leaf keys, and add an executing alias regression. The previous missing-path report concerns ordinary leaf metadata; this failure is in NESTED owner resolution.
FE UT Coverage ReportIncrement line coverage |
### Background SEARCH dependencies can be renamed or replaced with extracted Variant slots during predicate pushdown. ### What problem does this PR solve? Issue Number: None Related PR: apache#69344 PR Type: Bugfix Problem Summary: NESTED queries through projected Variant aliases retained the logical array owner even after their leaves were bound to a physical scan column. The owner could not bind to the read schema. Nested leaf mapping and document-count callbacks also used logical paths and could miss deeper physical groups. ### Details - **Frequency:** NESTED over a renamed Variant root or projected Variant prefix. - **Root cause:** nested_path and nested traversal callbacks did not follow the final physical field bindings. - **Why existing tests missed it:** Owner and leaf names matched scan names; alias tests covered ordinary SEARCH rather than NESTED serialization and nested callbacks. - **Fix:** Derive the physical nested owner from a normalized leaf binding during serialization, and translate callback paths through the same binding metadata in BE. - **Other paths and branches:** Cover materialized slots and retained element_at children, root aliases and projected prefixes, and both nested leaf mapping and row counts. Logical DSL lookup keys remain unchanged. - **Behavior change:** Aliased NESTED predicates use the final physical Variant owner and paths. ### Testing - Old code: three new FE tests failed with doc.items instead of v.items/v.payload.items; the new BE test failed with doc.items.tags.name instead of v.payload.items.tags.name. - Fixed code: 40 FE tests and 180 BE tests passed using run-fe-ut.sh and run-be-ut.sh. - Built FE and ASAN BE with build.sh and passed FE Checkstyle, clang-format 16, clang-tidy, and BE build hygiene. - Deployed FE/BE libraries, verified checksums, and passed all 19 targeted SEARCH/score/Variant regression suites with zero failures or skips. - Full NESTED result-set execution remains unverified because the available community build lacks the NestedGroup module. ### Release note Fix physical Variant path binding for NESTED SEARCH predicates through projected aliases. ### Check List (For Author) - Behavior changed: Yes (see Details). - Does this need documentation: No. - Pre-submission status: Partially verified: full NESTED result-set execution requires a NestedGroup-capable build; the available community build does not provide that module. ### AI Disclosure - Tool: OpenAI Codex. - Generated: Code, tests, and this description. - Human review: Pending.
|
/review |
There was a problem hiding this comment.
Static review of #69344 at 9fe2b52: no new actionable findings. Review converged in round 1. Complete FE and BE reviewers and a separate NESTED risk reviewer all returned NO_NEW_VALUABLE_FINDINGS; all 32 changed paths and related call chains were covered, and all candidates and risk focuses have evidence-backed dispositions.
Existing review context: all six prior reports are addressed at this head. The P1 cache report (4488661491) is fixed by including deterministic logical-to-physical storage bindings before bitmap-cache lookup. The P2 reports for executable constant substitution (4488661665), computed DSL/options folding (4488662116), missing-path Variant alias metadata (4488662248), INTERSECT/EXCEPT sibling inference (4488662377), and NESTED physical owner resolution (4489149834) have matching fixes in the current code. None is reposted; no independently confirmed existing P0/P1 remains blocking.
Critical checkpoint conclusions:
- Goal and proof: binding now exposes SEARCH dependencies before subquery expansion/pushdown, with final OLAP scan/index checks. Unit and regression sources cover CTE/NOT IN, aliases, joins and set operations; assertions were inspected, not executed.
- Scope and reuse: changes stay within SEARCH binding, validation, physical metadata/cache identity and their tests; existing substitution, folding and index checks are reused.
- Concurrency and locks: no new threads, shared mutation or locks; cache-key inputs and callback mappings are query scoped. No changed lock ordering or work under locks was identified.
- Lifecycle and static initialization: resolver-owned bindings back its pointers, borrowed scan/provider objects remain in scope, and callback cleanup is guarded. No new cross-translation-unit static dependency.
- Configuration: no new configuration or dynamic-setting contract.
- Compatibility: existing optional SEARCH metadata is reused; no persisted-format or plugin ABI change identified. Mixed-version runtime behavior was not executed.
- Thrift/protobuf: no gensrc definitions, field IDs or enum values change.
- Parallel paths: extracted slots and retained element_at paths, scalar and Variant fields, ordinary and NESTED callbacks, scoring/cache paths, and CTE/project/join/set-operation substitutions were inspected. Updated signature callers are consistent.
- Conditional checks and failures: new checks have concrete placement, constant-argument or physical-path invariants and explanatory comments. Unsupported executable shapes/index absence fail explicitly; eliminated SEARCH predicates avoid index checks.
- Test coverage: FE tests cover binding, ambiguity/missing fields, invalid placement, empty DSL, index checks, constants and inference. BE tests cover cache mapping/order isolation and both physical NESTED callbacks. Regression sources include result-set and negative cases; complex CTE and several new shapes use EXPLAIN.
- Changed results: binding_results.out agrees with inserted rows: city/country select 1/2, constant cases select 1, set-operation cases produce hello, and absent-city alias/direct cases select 1. Generation provenance was not independently reproduced.
- Observability: existing binding diagnostics, explicit errors and cache/timing statistics cover these paths; no new distributed operation requiring added metrics/logs.
- Transactions and persistence: no transaction, EditLog/replay, failover or persisted metadata write path changes.
- Data writes and atomicity: production changes affect query planning/index reads/cache identity; no new data mutation or crash-recovery protocol.
- FE/BE propagation: final physical parent/subpath metadata accompanies logical field keys; NESTED owner and both descendant callbacks use physical paths. No new wire variable is required.
- Performance and memory: binding occurs once during analysis; deterministic signature construction sorts by field count, not row count. No new material allocation, ownership leak, redundant row-wise work or memory-accounting defect substantiated by inspection.
- Other concerns: temporary NULL substitutions leave nonfoldable SEARCH unevaluated, so the suspected erroneous child NOT NULL inference was dismissed. No additional substantiated issue remained after the final sweep.
User focus: no additional focus was supplied; the whole PR was reviewed.
Validation limits: static inspection only; no builds, tests, benchmarks, source edits, submodule initialization or runtime validation were performed. The original complex sparse-Variant CTE regression is plan-only. Full enterprise NestedGroup result execution and its unavailable provider implementation remain outside the available evidence; serialization/callback tests do not prove those runtime results. These are explicit validation limits, not unresolved code candidates or uncovered changed paths.
Review converged after 1 round(s).
|
run buildall |
### Background Search predicates depend on source-column indexes and analyzers even when column values are equivalent. ### What problem does this PR solve? Issue Number: None Related PR: apache#69344 PR Type: Bugfix Problem Summary: INTERSECT and EXCEPT can propagate MATCH or multi_match predicates to sibling columns with different analyzers, changing query results. Constant propagation can also replace multi_match column arguments with literals and leave an expression that cannot execute through an inverted index. ### Details - **Frequency:** Search predicates across set-operation branches, or multi_match with an equality fixing a searched column. - **Root cause:** Equivalent-replacement inference enumerated search classes, while sibling insertion excluded only SearchExpression. ConstantPropagation protected Match and SearchExpression but omitted MultiMatch. - **Why existing tests missed it:** Inference cases did not combine equal-valued columns with different analyzers; multi_match was not tested with constant-equality filters. - **Fix:** Add NonInferableExpression to Match, MultiMatch, Search and SearchExpression, and use that trait at all three inference admission checks. Separately preserve MultiMatch column arguments during constant propagation. - **Other paths and branches:** Preserve ordinary predicate inference, alias substitution, and normal pushdown. Tests cover JOIN variants, INTERSECT/EXCEPT, expression wrappers, and a custom trait-bearing expression. - **Behavior change:** Prevent incorrect search predicate propagation and retain executable multi_match scan dependencies. ### Testing - Reproduced wrong INTERSECT/EXCEPT results for MATCH and multi_match on equal-valued columns with different analyzers before the fix. - Reproduced the multi_match constant-propagation execution error before the fix. - 164 FE tests passed using run-fe-ut.sh across six inference, constant-propagation, and SEARCH test classes. - FE build and Checkstyle passed using build.sh --fe; deployed the resulting FE library and verified its checksum. - Generated the new regression .out with run-regression-test.sh, checked all eight results, then passed 21 targeted regression suites with zero failures or skips. ### Release note Fix incorrect MATCH and multi_match results across INTERSECT/EXCEPT branches with different analyzers, and fix multi_match execution errors when an equality fixes a searched column. ### Check List (For Author) - Behavior changed: Yes (see Details). - Does this need documentation: No. - Pre-submission status: Ready. ### AI Disclosure - Tool: OpenAI Codex. - Generated: Code, tests, and this description. - Human review: Pending.
|
/review |
There was a problem hiding this comment.
Request changes for one new P1: ordinary pushdown of an outer SEARCH into EXCEPT's subtracting input can turn an empty result into a returned row when the inputs use different index analyzers. The inline includes a concrete query and reduced plan. This is distinct from the earlier inner-branch predicate-inference report, whose specific route is fixed.
Static review covers all 42 changed files at 7846086ee011db502cd1be13da4412238568fb80, the relevant planner-to-storage invocation chains, and the existing review threads. No additional user-provided focus was specified.
| Required checkpoint | Conclusion |
|---|---|
| Goal and supporting proof | Early slot binding addresses the CTE/NOT IN dependency problem and admits aliases/joins/UNION. Added planning and result tests support those paths, but the new outer-EXCEPT case remains incorrect. |
| Scope and clarity | Changes stay within SEARCH binding/validation, related index-dependent predicate inference, score adaptation, and physical binding/cache handling. |
| Concurrency and locking | No new threads, locks, shared metadata mutation, or blocking work under metadata locks was introduced. Rewrites and signature construction use query-local state. |
| Lifecycle and initialization | No new static initialization dependency or ownership cycle found. Nested callback cleanup and cache-handle lifetimes remain scoped; physical column pointers remain borrowed from the scan schema. No new memory-safety or unchecked-Status defect confirmed. |
| Configuration | No configuration item added; existing cache, scoring and folding controls remain in use. |
| Compatibility | No persisted-format or wire-definition change. Cache signature changes cause misses. Existing optional binding metadata now carries final physical paths. Mixed-version execution was not run. |
| Thrift/protobuf IDs | No gensrc/thrift or gensrc/proto changes. |
| Parallel paths | Checked CTE producers/consumers, subqueries, projections, joins, UNION, INTERSECT/EXCEPT, direct/extracted Variant fields, score binding, missing paths and NESTED callbacks. Direct outer EXCEPT pushdown is the uncovered correctness case. |
| Conditional checks | New branches have concrete missing/ambiguous/unsupported-input failure paths; comments explain delayed checks, alias identity and temporary inference replacements. |
| Test coverage | Inspected positive and negative FE/BE tests plus regression suites for binding, placement, indexes, constant arguments/equalities, analyzer-dependent inference and cache isolation. Add an executing outer-EXCEPT regression for the P1. |
| Expected results | Both new .out files agree with their fixtures: city/country cache queries select rows 1/2, missing-city paths add no matches, and inner-branch set-operation/analyzer checks have consistent results. Generation claims in the PR description were not independently executed. |
| Observability | Existing field/index analysis errors, logical/physical binding diagnostics, cache counters and query timers remain available; no additional instrumentation requirement identified. |
| Transactions and persistence | No EditLog, commit/replay, visible-version or delete-bitmap mutation; no new failover requirement. |
| Data writes and crash atomicity | Production write paths are unchanged. Result bitmap ownership and cache copies remain intact. |
| FE/BE variable propagation | No new transmitted field. Final descriptor parent/subpath metadata, physical nested callbacks and storage bindings were traced through their consumers. |
| Performance | Binding is per filter; signature sorting is per segment/query over referenced fields. No new per-row copy, unbounded retry or material redundant work identified. |
| Other issues | No additional verified finding after the repeated missed-path checks. |
Earlier reports were revalidated against this code and are addressed: physical Variant cache identity (4488661491), executable constant propagation (4488661665), computed DSL/options (4488662116), extracted-alias missing-path metadata (4488662248), inner-branch sibling inference (4488662377), and NESTED physical-owner binding (4489149834). They are not reposted; no existing P0/P1 was confirmed still applicable.
Validation was static only: no builds, tests, benchmarks, source edits, or submodule initialization. The original sparse-Variant CTE regression is explicitly EXPLAIN-only; full NestedGroup execution and runtime compatibility remain unverified. These are execution limits, with no unresolved static candidate beyond the accepted P1.
Converged after 2 rounds: all three latest reviewer results are NO_NEW_VALUABLE_FINDINGS, required static coverage is complete, and every candidate/risk focus has an evidence-backed disposition. One new P1 is accepted; no unresolved candidate or uncovered changed path remains.
Review converged after 2 round(s).
| @Override | ||
| public Rule build() { | ||
| return logicalFilter(logicalOlapScan()) | ||
| return logicalFilter() |
There was a problem hiding this comment.
[P1] Preserve EXCEPT subtraction when pushing down outer SEARCH
The generalized binding now admits this reduced plan:
Filter(SEARCH(label:hello))
Except
Project(title AS label) -> Scan(t, title index: unicode)
Project(body) -> Scan(t, body index: keyword)
Using the new test_search_non_inferable fixture (both columns are VARCHAR(100) and both contain hello world), with spill disabled:
SELECT label FROM (
SELECT title AS label FROM search_non_inferable
EXCEPT SELECT body FROM search_non_inferable
) s WHERE search('label:hello');The inner EXCEPT is empty, so the result must stay empty. However, PushDownFilterThroughSetOperation.addFiltersToNewChildren copies the bound SEARCH into both branches and removes the outer filter; project substitution maps its children to title/body. The Unicode index matches token hello, while the keyword index contains only hello world, so the rewritten EXCEPT returns hello world. Both final scan predicates pass index validation.
This is a separate route from the earlier inner-branch inference report: the new NonInferableExpression checks in InferPredicates never run for this ordinary pushdown. Preserve the subtracting input when pushing this predicate (or reject this unsupported outer placement), and add a result regression for the outer-SEARCH form.
### Background Search predicates depend on the source column's index and analyzer, not only on its value. Set-operation pushdown substitutes each branch's output columns. ### What problem does this PR solve? Issue Number: None Related PR: apache#69344 (follow-up) PR Type: Bugfix Problem Summary: Outer SEARCH predicates could be copied to branches with different analyzers, changing EXCEPT results. Rename NonInferableExpression to SourceDependentExpression and retain marked conjuncts above every set operation, including UNION ALL. Existing final SEARCH validation rejects unsupported placement. ### Details - **Frequency:** Outer SEARCH on set operations with source-dependent matching. - **Root cause:** Ordinary set-operation pushdown bypassed inference guards; UNION ALL had an unconditional fast path. - **Why existing tests missed it:** Tests expected outer UNION SEARCH to be copied into each branch. - **Fix:** Classify all conjuncts using the source-dependent marker while preserving existing volatile-expression handling. - **Other paths and branches:** Inference guards and implementations use the renamed interface. Same-source alias pushdown remains allowed. MATCH/MultiMatch final validation is deferred. - **Behavior change:** Outer SEARCH retained above set operations is rejected by existing FE validation; branch-local SEARCH remains supported. ### Testing On ssh dev: 160 FE unit tests passed across CheckSearchUsageTest, PushDownFilterThroughSetOperationTest, InferPredicateByReplaceTest and InferPredicatesTest. Standard build.sh --fe with DISABLE_BUILD_UI=ON passed, including Checkstyle. Updated FE deployed. Generated regression output using run-regression-test.sh. SEARCH directory: 47/48 suites passed, including changed suites. test_search_inverted_index_profile failed and failed independently on retry because InvertedIndexSearcherCacheHit was 0 instead of positive; its single-table query does not exercise set-operation pushdown. ### Release note SEARCH predicates over set-operation outputs are rejected instead of being evaluated with different branch indexes or analyzers. SEARCH inside individual branches remains supported. ### Check List (For Author) - Behavior changed: Yes (see Details) - Does this need documentation: No - Pre-submission status: Partially verified: one cache-profile regression remains failing as described in Testing. ### AI Disclosure - Tool: Codex - Generated: Code, tests, and commit description. - Human review: Pending.
|
/review |
TPC-H: Total hot run time: 28315 ms |
TPC-DS: Total hot run time: 154280 ms |
ClickBench: Total hot run time: 24 s |
|
Codex automated review failed and did not complete. Error: Review recovery stopped: shared time budget or helper timeout exhausted Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
|
/review |
|
Codex automated review failed and did not complete. Error: Review setup stopped before context preparation ran; inspect the first failed or cancelled setup step. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
|
/review |
|
Codex automated review failed and did not complete. Error: Review setup stopped before context preparation ran; inspect the first failed or cancelled setup step. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
|
/review |
There was a problem hiding this comment.
Request changes: the existing P1 outer SEARCH/EXCEPT finding still applies to head 7846086ee011db502cd1be13da4412238568fb80. Early binding exposes the set-output slot, and ordinary PushDownFilterThroughSetOperation copies SEARCH into every EXCEPT input. With equal hello world strings, the Unicode title branch matches hello while the keyword body branch does not, so filtering an empty EXCEPT can incorrectly produce a row. The new inference guards do not protect this ordinary pushdown. This invocation adds no duplicate inline finding.
All 42 changed paths were reviewed statically by the main reviewer and two complete-scope reviewers, with an additional independent reviewer for the identified risks. All three returned NO_NEW_VALUABLE_FINDINGS; no candidate or risk question remains unclassified. The earlier cache-identity, constant-propagation, computed-argument, missing-Variant-path, sibling-inference and NESTED-owner reports are addressed at this head.
| Critical checkpoint | Conclusion |
|---|---|
| Goal and evidence | Early slot binding supports CTE/NOT IN, visible aliases and UNION dependencies; inspected FE/regression tests cover these shapes. The existing EXCEPT correctness defect prevents approval. |
| Scope, clarity and reuse | Changes stay within binding, validation, inference and physical field identity. The shared non-inference trait replaces duplicated concrete-type checks. |
| Concurrency and locking | Query-local rewrites introduce no threads or new shared mutable state/locks. Existing cache synchronization is reused; no new lock-order or heavy-lock work identified. |
| Lifecycle and initialization | Resolver owns copied bindings; nested callbacks are reset before borrowed readers/providers expire. No new ownership cycle or cross-translation-unit static dependency. |
| Configuration | No new configuration or dynamic-refresh obligation. |
| Compatibility | Existing wire fields are reused; no persistent format change. Updated BE accepts direct-name bindings; final FE metadata supplies physical aliases. Cache signatures intentionally separate storage identities. Mixed-version runtime behavior was not executed. |
| Thrift/protobuf IDs | No definition, field-ID or enum-value change. |
| Parallel paths | Checked MATCH/multi_match/SEARCH, both sibling-inference admission paths, SCORE binding, extracted/retained Variant paths and shared cloud/local query paths. Ordinary set-operation pushdown remains the known exception above. |
| Conditional checks and errors | Constant-string, unique visible-field, Variant-parent, final scan-shape and index checks have concrete failure cases. FE AnalysisException and existing BE Status conversion remain in use. |
| Test coverage | Inspected positive/negative unit and regression coverage for aliases, CTE thresholds, joins, UNION, empty DSL, index timing, cache isolation, absent paths and inference. Outer SEARCH/EXCEPT needs the regression described in the existing finding. |
| Changed expected results | Both new .out files (16 binding sections and eight non-inference sections) agree with their fixtures and analyzer semantics by static inspection. |
| Observability | Existing field-specific analysis errors and logical/storage binding diagnostics remain useful; no additional critical metric requirement identified. |
| Transactions and persistence | No EditLog, transaction, failover or persistent metadata modification. |
| Data writes and atomicity | No write-path change. Visible-version and delete-bitmap invariants are unaffected; query result correctness remains affected by the existing EXCEPT issue. |
| FE/BE propagation | Traced final descriptor/element_at metadata through Thrift, vsearch input collection, cache signatures and NESTED owner/leaf callbacks. No new transmitted variable or omitted sending path identified. |
| Performance and memory | Cache binding canonicalization costs O(F log F) for F fields with local copies. No substantial new allocation, ownership leak or demonstrated hot-path regression identified; no performance measurements run. |
| Other invariants | Preserved SEARCH bitmap/NULL handling, checked temporary NULL substitutions and final executable-child validation. No BE include/PCH/template/unity or new libc dependency change. |
No additional user focus was supplied. Validation limits: this was a static-only review; no builds, tests, benchmarks or SQL execution ran. The original complex sparse-Variant CTE case is mainly covered by EXPLAIN, and full enterprise NestedGroup result execution remains unverified; the new tests inspect serialization and callbacks. These runtime limits are distinct from completed static coverage. Review coverage converged in one round, with the existing P1 retained and no new findings.
Review converged after 1 round(s).
Existing P0/P1 findings confirmed for this head: #69344 (comment)
|
/review |
|
run buildall |
There was a problem hiding this comment.
Static review of head b5be6ee found one new P2: the blanket source-dependent set-operation guard rejects existing MATCH queries above UNION ALL even when both branches use the same indexed source. The inline finding gives the query and rewrite chain. No new P0/P1 was verified.
All seven existing threads were independently rechecked and are addressed at this head: Variant cache identity (4488661491) now includes final physical metadata and sorted storage bindings; executable constant propagation (4488661665) preserves SEARCH/multi_match dependencies; computed arguments (4488662116) fold before parsing; extracted Variant aliases (4488662248) retain physical metadata; sibling inference (4488662377) is trait-guarded; NESTED aliases (4489149834) remap physical owners and callbacks; outer SEARCH over EXCEPT (4490192381) is retained and rejected instead of changing subtraction. Neither prior P1 remains applicable, so existing_blocking_comment_ids is empty. No duplicate finding is posted.
The required checkpoints were assessed as follows:
| Checkpoint | Conclusion |
|---|---|
| Goal and proof | Early visible-slot binding precedes subquery expansion and preserves CTE/NOT IN dependencies. Added positive and negative tests exercise this, but the new MATCH compatibility regression remains. Tests were inspected only. |
| Scope and clarity | Changes stay within SEARCH binding, source-preservation rules, physical field mapping, cache identity and their tests; the broad set-operation guard needs refinement. |
| Concurrency and locking | No new thread entry, shared-state synchronization or lock ordering is introduced. Evaluation-local maps and existing cache handles retain their ownership model. |
| Lifecycle and static initialization | Resolver binding storage and synchronous callback lifetimes remain valid; existing scoped reset and cache-handle cleanup remain intact. No new cross-translation-unit static dependency. |
| Configuration | No configuration items are added. Existing folding/cache/scoring settings are respected in the inspected paths. |
| Compatibility | Existing thrift fields are reused; no persisted format changes. Same-source MATCH-over-UNION-ALL SQL compatibility regresses as reported. Mixed-version execution was not run. |
| Thrift/protobuf IDs | No gensrc/thrift or gensrc/proto definitions change. |
| Parallel paths | MATCH, multi_match, scalar/bound SEARCH, sibling JOIN/set inference, aliases, spill conversion and Variant materialized/element_at paths were traced. The shared pushdown guard causes the reported regression. |
| Conditions and failures | Added conditions explain source/index or physical-path assumptions; unsupported executable SEARCH shapes fail analysis. The retained MATCH fails the existing final scan-placement check. |
| Test coverage | FE, BE and regression sources cover binding, ambiguity, invalid placement, eliminated predicates, constants, cache isolation, missing paths and set inference. Positive outer MATCH-over-UNION-ALL coverage is missing. |
| Changed expected results | Both new .out files were checked against fixture rows, analyzer semantics and ordering; their expected results are consistent. Generation provenance was not independently established. |
| Observability | Existing analysis errors, index binding diagnostics and cache counters cover these paths; no additional logging or metrics requirement was identified. |
| Transactions and persistence | No transaction, EditLog, replay or master-failover behavior changes. Committed-version visibility and MoW delete-bitmap handling are untouched. |
| Writes and atomicity | No data-write, publish, compaction or crash-recovery path changes. |
| FE/BE values | Final physical Variant parent/path is serialized for extracted slots and retained element_at; NESTED owner/leaf mapping and storage-aware cache identity were traced through BE consumers. |
| Performance and memory | Stable cache identity adds O(F log F) sorting for F bound fields per lookup. No new long-lived allocation, ownership cycle or unbounded loop was found; no benchmark was run. |
| Other issues | No additional substantiated issue after the missed-issue recheck. |
No additional user focus was supplied. All changed files, existing threads and risk items have dispositions; there is no remaining static changed-file coverage gap. Validation is limited to source/diff/test-output inspection: no build, tests, sanitizer, benchmark, final ELF audit, mixed-version cluster or enterprise NestedGroup execution ran. Enterprise NestedGroup internals are outside this checkout; the available serialization/callback code and tests were inspected. Author-reported runtime results are not independent validation.
Review converged after 2 round(s).
| Set<Expression> keptAboveConjuncts = new LinkedHashSet<>(); | ||
| for (Expression conjunct : origFilter.getConjuncts()) { | ||
| // Equal values in different branches may use different indexes or analyzers. | ||
| if (conjunct.containsType(SourceDependentExpression.class) |
There was a problem hiding this comment.
[P2] Preserve MATCH pushdown through same-source UNION ALL
Match now implements SourceDependentExpression, so this guard also strands previously supported MATCH predicates above UNION ALL. For example, with an inverted index on t.title:
SELECT label FROM (
SELECT title AS label FROM t WHERE id = 1
UNION ALL
SELECT title FROM t WHERE id = 2
) s WHERE label MATCH 'hello';The reduced tree is Filter(Match(label)) -> UnionAll(Project(title AS label) -> Scan(t), Project(title) -> Scan(t)). Previously, addFiltersToNewChildren and project substitution moved MATCH to both scans. Now the rule retains it above the union, and CheckAfterRewrite.checkMatchIsUsedCorrectly throws only support in olapScan filter. Both branches use the same physical column/index/analyzer, so the previous pushdown preserves source semantics. Preserve this safe existing case and add a positive MATCH-over-UNION-ALL regression. This is distinct from the earlier outer-SEARCH/EXCEPT finding, which involved different analyzers and incorrect subtraction.
There was a problem hiding this comment.
Thanks for pointing this out. I agree that pushing MATCH into both UNION ALL branches is safe in this example because both branches read the same indexed column. This PR keeps the conservative behavior, so the outer MATCH in your example will be rejected. I'll defer support for same-source MATCH pushdown through UNION ALL to a follow-up, where we can verify the column source in every branch and add a positive regression test.
What problem does this PR solve?
SEARCH combined with a NOT IN subquery inside a CTE can fail analysis after subquery expansion introduces an Apply. SEARCH fields are initially embedded in a DSL string, so predicate pushdown cannot see their column dependencies until they are bound. Requiring a single-scan shape before rewriting also rejects predicates that could reach a valid scan through a join, projection or UNION.
Bind SEARCH fields top-down to the filter's visible input slots during analysis, before subquery expansion and predicate pushdown. Each field must resolve to exactly one input column. Existing rewrite rules can then substitute aliases, map UNION branches and push predicates using their actual column dependencies.
Consolidate early SEARCH validation in CheckAnalysis: SEARCH is allowed only in filter predicates, and empty or whitespace-only DSL reports
Invalid search expression: empty search DSL. Remove the standalone CheckSearchUsage rule. After rewriting, require every surviving SEARCH to be in a filter directly above an OLAP scan, reference supported scan-column expressions and have the required inverted indexes. Variant subcolumn checks use the original parent column. SEARCH eliminated during rewriting no longer requires an index. Allow temporary expression-child replacements used by optimizer inference, with executable children validated by the final check.The new regression suites use EXPLAIN to validate FE plans for inline, materialized and reused CTEs, NOT IN, joins, projected and shadowed aliases, Variant paths and UNION branches. Negative cases validate field resolution, placement, empty DSL and index errors. Additional result-set regression covers aliased Variant cache isolation, absent-path segments, constant equalities and computed SEARCH arguments, and INTERSECT/EXCEPT with unindexed sibling columns.
Review follow-up preserves the final physical field bindings through these rewrites:
Release note
Fix SEARCH predicate handling with CTEs and NOT IN subqueries. Allow unambiguous SEARCH predicates over joins, projected aliases and UNION outputs when rewriting can push them to indexed OLAP scan columns. Reject empty SEARCH DSL with an explicit analysis error.
Check List (For Author)
Validation of the earlier review fixes (7ef8f50):
NESTED alias follow-up validation:
Non-inference trait and multi_match follow-up validation:
Reproduced incorrect MATCH/multi_match INTERSECT and EXCEPT results with equal strings indexed using different analyzers, and the multi_match execution error caused by replacing a searched column with an equality constant.
Added trait-based inference tests, set-operation/JOIN planning tests, and single-column/partially constant multi_match tests. All 164 FE tests passed.
FE build and Checkstyle passed. Deployed the FE library and verified its checksum. Generated the new regression .out with the test script, inspected the eight results, and passed all 21 targeted regression suites with zero failures or skips.
The new trait only controls predicate inference; ordinary pushdown, alias substitution, and pruning keep their existing behavior. ConstantPropagation separately preserves multi_match column arguments.
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)