[refactor](search) Simplify Variant search iterator binding - #66449
Merged
eldenmoon merged 1 commit intoAug 5, 2026
Merged
Conversation
### What problem does this PR solve? Issue Number: None Related PR: apache#60847, apache#63660 Problem Summary: The scanner materializes Variant path slots, and the storage layer resolves direct or inherited index metadata before constructing an iterator for that child scan column. VSearch additionally tried to resolve a physical parent ColumnId, borrow the parent iterator, and synthesize the stored field name when the child iterator was absent. That duplicated storage ownership, mixed scan column indexes with tablet ColumnIds, and could bypass the path and index selection used by NestedGroup. Remove the expression-level parent rebinding, use only the iterator associated with the SlotRef scan column, remove the fallback-only IndexExecContext APIs, and add a focused parent-present child-missing test. ### Release note Variant SEARCH no longer borrows a parent Variant iterator when the materialized child iterator is absent. ### Check List (For Author) - Test: Unit Test and Regression Test - ASAN BE build: ./build.sh --be - Unit Test: 60 focused VSearch and NestedGroup provider tests passed - Regression Test: variant_p0/test_variant_search_subcolumn passed - Static analysis: run-clang-tidy was attempted; repository core/types.h NOLINT parsing and system stddef.h discovery errors prevented completion - Behavior changed: Yes. A missing Variant child iterator now produces an empty index result instead of being reinterpreted through the parent iterator. - Does this need documentation: No
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Member
Author
|
run buildall |
1 similar comment
Member
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 28734 ms |
Contributor
TPC-DS: Total hot run time: 165897 ms |
Contributor
ClickBench: Total hot run time: 23.8 s |
eldenmoon
marked this pull request as ready for review
August 5, 2026 07:44
eldenmoon
changed the title
Member
Author
|
/review |
Contributor
There was a problem hiding this comment.
Pull request overview
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Refactors Variant SEARCH iterator binding to rely solely on the SlotRef’s scan-column iterator, removing the expression-level fallback that borrowed a parent Variant iterator when a materialized subcolumn iterator is missing.
Changes:
- Removed parent-iterator rebinding logic for Variant subcolumns in
VSearchExprinput collection. - Introduced a helper to collect SlotRef-based search inputs using explicit scan column indexes.
- Added a focused unit test covering the “parent iterator exists, child iterator missing” boundary case.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| be/src/exprs/vsearch.cpp | Removes parent fallback logic; adds helper to bind SEARCH inputs strictly by scan column index + iterator presence. |
| be/src/exprs/vexpr_context.h | Deletes fallback-only IndexExecContext APIs used by the removed rebinding logic. |
| be/test/exprs/vsearch_expr_test.cpp | Adds a regression-style unit test ensuring missing Variant child iterators do not cause parent iterator usage. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
csun5285
approved these changes
Aug 5, 2026
airborne12
reviewed
Aug 5, 2026
Contributor
|
Codex automated review failed and did not complete. Error: All Codex review accounts are usage-limited; earliest retry is 2026-08-08T03:32:00Z. Please trigger /review again after that time. |
Member
Author
|
skip check_coverage |
github-actions
Bot
added
the
approved
Contributor
|
PR approved by at least one committer and no changes requested. |
Contributor
|
PR approved by anyone and no changes requested. |
github-actions Bot
pushed a commit
that referenced
this pull request
Aug 5, 2026
Related PR: #60847, #63660 Problem Summary: OlapScanner materializes a requested Variant path as a scan-schema column. For example, data.items.message gets its own SlotRef and scan column position. VariantColumnReader then resolves either direct subcolumn index metadata or metadata inherited from data, and SegmentIterator constructs the runtime iterator for that child column with the complete stored Variant path. VSearch duplicated that storage responsibility. When the child iterator was absent, it resolved the tablet ColumnId for data, borrowed the parent iterator, and synthesized a stored field name. This mixed scan column positions with tablet ColumnIds and could bypass the path and physical index selection already performed by the storage layer, including NestedGroup selection. This PR removes the expression-level parent rebinding. VSearch now consumes only the iterator attached to the SlotRef scan column. If that iterator is absent, the field remains an empty index input. It also removes the three fallback-only IndexExecContext APIs and separates SlotRef collection into a small helper that names the scan column index explicitly. A focused unit test constructs the exact boundary case: the data parent iterator exists, while the data.items.message child iterator does not. The test verifies that SEARCH does not mark or execute the parent iterator. ### Release note Variant SEARCH no longer borrows a parent Variant iterator when the materialized child iterator is absent.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Issue Number: None
Related PR: #60847, #63660
Problem Summary:
OlapScanner materializes a requested Variant path as a scan-schema column. For example, data.items.message gets its own SlotRef and scan column position. VariantColumnReader then resolves either direct subcolumn index metadata or metadata inherited from data, and SegmentIterator constructs the runtime iterator for that child column with the complete stored Variant path.
VSearch duplicated that storage responsibility. When the child iterator was absent, it resolved the tablet ColumnId for data, borrowed the parent iterator, and synthesized a stored field name. This mixed scan column positions with tablet ColumnIds and could bypass the path and physical index selection already performed by the storage layer, including NestedGroup selection.
This PR removes the expression-level parent rebinding. VSearch now consumes only the iterator attached to the SlotRef scan column. If that iterator is absent, the field remains an empty index input. It also removes the three fallback-only IndexExecContext APIs and separates SlotRef collection into a small helper that names the scan column index explicitly.
A focused unit test constructs the exact boundary case: the data parent iterator exists, while the data.items.message child iterator does not. The test verifies that SEARCH does not mark or execute the parent iterator.
Release note
Variant SEARCH no longer borrows a parent Variant iterator when the materialized child iterator is absent.
Check List (For Author)