Skip to content

[feature](lance) Push down SQL vector Top-N and BM25 search - #69346

Open
Gabriel39 wants to merge 8 commits into
apache:branch-4.2from
Gabriel39:dev/lance-search-expression-pushdown-4.2
Open

Gabriel39 wants to merge 8 commits into
apache:branch-4.2from
Gabriel39:dev/lance-search-expression-pushdown-4.2

Conversation

@Gabriel39

@Gabriel39 Gabriel39 commented Oct 20, 2026 •

Copy link
Copy Markdown
Contributor

What problem does this PR solve?

Related PR: #69332 (lance-c dependency upgrade)

Supersedes #69339; targets branch-4.2.

Ordinary SQL against Lance external tables does not use the existing Lance search path for vector Top-N, and score() cannot produce Lance BM25 relevance. Users currently need explicit vector_search() / full_text_search() TVFs.

This PR adds automatic rewrites on branch-4.2, reusing the existing fixed-snapshot search planner, index-segment execution, global candidate merge, and deferred payload reads. It directly uses lance_scanner_set_row_addr_allowlist() introduced by lance-c #96. The required lance-c revision 4fafcd64525b274b113208390bbcca86bbbc093b from #69332 is already included in the branch-4.2 base; this PR no longer duplicates the dependency upgrade.

Function and metric coverage

Doris SQL expression Lance metric / query Automatic pushdown in this PR SQL value and ordering
l2_distance_approximate(v, q) l2 Yes, ascending Top-N Euclidean distance sqrt(native squared L2), smaller is better
inner_product_approximate(v, q) dot (dot_product alias in the TVF) Yes, descending Top-N Inner product 1 - native dot distance, larger is better
l2_distance(v, q) / inner_product(v, q) Mathematically related to l2 / dot No; exact functions remain scalar Exhaustive scalar evaluation; no automatic ANN substitution
cosine_distance_approximate(v, q) (new) cosine Yes, ascending Top-N Native cosine distance, smaller is better
cosine_similarity_approximate(v, q) (new) cosine Yes, descending Top-N 1 - native cosine distance, larger is better
hamming_distance_approximate(v, q) (new) hamming Yes, ascending Top-N on UInt8 fixed-size-list vectors Number of different bits (XOR + popcount), smaller is better
cosine_distance(v, q) / cosine_similarity(v, q) / hamming_distance(v, q) (new exact Hamming function) Related to cosine / hamming No; exact functions remain scalar Exhaustive scalar evaluation with input validation
l1_distance(v, q) No L1 metric exposed by the bundled vector-search API No Remains scalar
score() with MATCH_ANY / MATCH_ALL / MATCH_PHRASE Prepared BM25 match-OR / match-AND / phrase Yes, descending Top-N Native Lance BM25 relevance, larger is better

This PR covers five vector SQL functions with the _approximate suffix: L2, inner product, cosine distance, cosine similarity, and Hamming distance. The three cosine/Hamming approximate functions and exact hamming_distance() are newly registered FE/BE scalar functions as well, so queries that cannot be rewritten have an executable scalar fallback. Supported multi-vector queries and tuning such as nprobes, refine_factor, and ef remain explicit-TVF capabilities. L1 is not exposed by the bundled Lance vector-search API and remains scalar.

The raw vector_search() TVF continues to return Lance _distance: squared L2 for l2, 1 - dot for dot, cosine distance for cosine, and Hamming distance for hamming. The automatic rewrite applies the conversions in the table; raw TVF output is unchanged.

Hamming accepts integer arrays up to BIGINT (including Lance UInt8 arrays exposed as SMALLINT), and validates every scalar element in [0,255] before computing bit differences. Floating-point, decimal, and LARGEINT arrays are rejected before implicit casts can truncate or wrap values. A binary vector of N bytes has 8N bits; Hamming counts differing bits, not unequal bytes. Automatic rewriting requires a physical UInt8 fixed-size-list column; arbitrary SQL casts or other physical element types keep scalar execution.

Exact cosine functions retain their current semantics. The new approximate cosine functions use the same scalar kernels when not rewritten, including zero-vector distance 2 and similarity 0. Automatic cosine rewriting requires a positive, finite Float32 query squared norm; zero, non-finite, overflowing, or underflowing query norms stay scalar. In eligible native searches, stored zero-norm vectors producing NaN distances are excluded along with other NaN candidates. Native cosine arithmetic and index estimates can differ numerically from scalar kernels; the approximate functions do not promise bit-identical values or exhaustive rankings.

BM25 values are computed from Lance's stored analyzer/index and pinned-dataset scoring statistics. This supports the SQL usage pattern used by Doris internal tables, but does not promise identical numeric scores or tokenization to a separately indexed Doris internal table. Fuzzy BM25, multi-column boolean SEARCH() expressions, custom analyzer overrides, and arbitrary score predicates are outside this automatic rewrite.

Approximate versus scalar semantics (including NULL / NaN)

Calling an _approximate function in an eligible Top-N opts into the native Lance search candidate contract. This PR does not add a separate session switch or scan all stored vectors merely to reproduce scalar validation.

Input / behavior Existing scalar execution (including exact functions) Eligible native approximate Top-N
Ordinary non-NULL finite Float32 vectors / valid UInt8 Hamming vectors Exhaustive scalar distances Native candidate retrieval, potentially approximate with a compatible index
Stored NULL vector Scalar distance raises a NULL-input error Native search excludes that vector
Stored vector producing a NaN distance Scalar distance can return NaN and Doris Top-N can retain the row Native search excludes NaN distances, including a single-row/all-NaN dataset
NULL element inside a stored vector Scalar distance raises a NULL-element error Unsupported input domain: callers must ensure vector elements contain no NULLs; native kernels do not reproduce scalar element-NULL validation
Exact L2 / inner product / cosine / Hamming functions Existing scalar evaluation Never rewritten by this rule
Projection containing a non-movable expression such as assert_true() Evaluation/errors remain before Top-N Rewrite is skipped to avoid suppressing evaluation on pruned rows

For example, for rows (1, NULL) and (2, [1,0]), a native approximate search can return only row 2; the corresponding scalar distance query raises an error. For a dataset containing only [NaN,0], native approximate search returns no candidates whereas scalar Top-N can retain its NaN-valued row. Fewer than LIMIT rows is therefore possible when vectors do not belong to the native candidate domain. These are deliberate, documented semantics of opting into provider-native approximate search, not claims that the old scalar NULL/NaN behavior is preserved. Use exact functions when scalar validation and row eligibility are required.

SQL examples and behavior changes

The examples assume a Lance table with a three-dimensional Float32 embedding whose elements contain no NULLs, and a supported full-text index on body.

1. Approximate vector Top-N

SELECT doc_id,
       l2_distance_approximate(embedding, [0.1, 0.2, 0.3]) AS distance
FROM lance_catalog.docs.documents
WHERE tenant_id = 7
ORDER BY distance ASC
LIMIT 10 OFFSET 5;

Before: ordinary Lance scanning evaluates the scalar distance and Doris performs Top-N; this SQL does not request native Lance vector search.

After: the optimizer creates a native Lance L2 search with the original scalar predicate applied before candidate selection. Each search split requests 15 candidates with offset 0; Doris merges them and applies the final global OFFSET 5 LIMIT 10. Compatible Lance vector indexes can serve the approximate search through the existing search planner. Candidate selection may therefore be approximate rather than an exhaustive scalar ranking.

The SQL result remains Euclidean distance: Lance's squared L2 result is converted with CAST(sqrt(_distance) AS FLOAT).

Inner-product queries use the corresponding descending order:

SELECT doc_id,
       inner_product_approximate(embedding, [0.1, 0.2, 0.3]) AS similarity
FROM lance_catalog.docs.documents
WHERE tenant_id = 7
ORDER BY similarity DESC
LIMIT 10;

This becomes native Lance dot search. The projection converts Lance dot distance with 1.0f - _distance, so the SQL output remains an inner-product similarity and higher is better.

Cosine uses the same native metric for both SQL functions, with different result conversion and ordering:

SELECT doc_id, cosine_distance_approximate(embedding, [0.1, 0.2, 0.3]) AS distance
FROM lance_catalog.docs.documents
ORDER BY distance ASC LIMIT 10;

SELECT doc_id, cosine_similarity_approximate(embedding, [0.1, 0.2, 0.3]) AS similarity
FROM lance_catalog.docs.documents
ORDER BY similarity DESC LIMIT 10;

Hamming queries target binary UInt8 vectors. For example, on a three-byte embedding:

SELECT doc_id, hamming_distance_approximate(binary_embedding, [0, 255, 170]) AS distance
FROM lance_catalog.docs.binary_documents
ORDER BY distance ASC LIMIT 10;

-- Exact scalar evaluation, without automatic ANN substitution:
SELECT hamming_distance([0, 255, 170], [255, 0, 85]); -- 24 differing bits

Before this addition, the three new approximate names and exact Hamming name were unavailable in SQL. Afterward they work as scalar functions and eligible approximate Top-N queries automatically use Lance cosine/Hamming retrieval. Existing exact cosine expressions continue to use scalar evaluation.

2. BM25 full-text Top-N

SELECT doc_id, score() AS relevance
FROM lance_catalog.docs.documents
WHERE body MATCH_ANY 'database search'
  AND tenant_id = 7
ORDER BY relevance DESC
LIMIT 10;

Before: score() on this Lance external scan is unsupported.

After: one top-level MATCH predicate becomes a native prepared Lance full-text query, score() reads its generated BM25 score, and Doris merges the highest-scoring candidates. The native prepared query preserves the dataset-wide scoring context across index segments and row-ID filtering.

  • MATCH_ANY maps to a match query with OR.
  • MATCH_ALL maps to a match query with AND.
  • MATCH_PHRASE maps to a phrase query using the stored analyzer and position information.
  • A lower score bound such as AND score() >= 0.5 is supported as a residual over the generated score. Upper bounds and other score predicates are rejected because filtering a pruned Top-K with those predicates could lose valid results.

3. Scalar WHERE to native row-ID prefilter

For the scalar predicates in the examples, the execution path is:

Bound Lance snapshot + validated Substrait predicate
    -> row-ID-only native scan (no user-column projection)
    -> native UINT64 _rowid allowlist
    -> vector / prepared full-text search
    -> Doris global Top-N and final projection

The candidate scanner and search scanner use the same pinned dataset snapshot. A query-local cache shares the candidate list across search readers using the dataset URI, version, and filter as its key. An empty allowlist selects zero rows.

The list is capped at 65,536 IDs. Broader predicates discard the entire partial list and retain the complete native Substrait prefilter on the search scanner. This bounds the C++ candidate list and preserves correctness. Profile counters expose prefilter time, candidate count, cache hits, and fallback count.

ACL / EXISTS example and the remaining boundary

SELECT d.doc_id,
       d.title,
       l2_distance_approximate(d.embedding, [0.1, 0.2, 0.3]) AS distance
FROM lance_catalog.docs.documents d
WHERE EXISTS (
    SELECT 1
    FROM internal.app.document_acl a
    WHERE a.doc_id = d.doc_id
      AND a.user_id = 1001
)
ORDER BY distance ASC, d.doc_id ASC
LIMIT 10;

This query does not receive automatic Lance Top-N pushdown in this PR. It contains an ACL semi-join and a secondary sort key, outside the direct-scan/single-key rewrite. It retains the existing join/filter and scalar-distance/global-Top-N execution. Using exact l2_distance() instead likewise remains scalar.

The native row-ID allowlist added here is produced from fully pushable predicates on the same pinned Lance dataset. It does not translate an ACL table's business doc_id values into Lance _rowid, and it does not consume dynamically produced semi-join IDs. Supporting that scenario requires a separate doc_id -> native _rowid lookup plus a join/runtime-filter handoff, with ACL filtering before candidate pruning. Pushing an unfiltered Top-K below EXISTS could miss the nearest authorized documents, so this PR deliberately avoids that rewrite.

Scope and compatibility

  • Automatic rewriting requires a direct Lance scan, an optional filter, a projection, and a single search ordering key with LIMIT. It does not rewrite across JOIN/EXISTS, add secondary sort keys, or support the internal-table SEARCH() DSL.
  • Vector rewriting supports physical Float32 fixed-size-list vectors for L2/dot/cosine and UInt8 fixed-size-list vectors for Hamming, with constant query arrays. Exact distance/similarity functions, reversed vector ordering, incompatible vector types, and vector queries with unsupported residual predicates keep scalar execution.
  • BM25 requires one supported top-level MATCH on a direct string column, a literal query, the stored analyzer, and ORDER BY score() DESC. Remaining scalar predicates must be fully convertible by the existing Lance predicate converter. Unsupported scored-query shapes fail explicitly rather than silently selecting incorrect candidates.
  • The rewrite preserves bound snapshot/version metadata and output expression IDs, retains column pruning, and disables SQL result caching for rewritten searches.
  • Row-ID collector and cached allowlist buffers use Doris tracked allocation, so their retained capacity participates in query memory limits and is released when the query-local cache is destroyed.
  • Row-ID prefiltering honors both query cancellation and scanner-local early stop, including downstream JOIN short-circuiting. Interrupted collections return stopped EOF, release scanner/stream resources, and never enter the shared cache; an active reader can recompute the complete mask.
  • Deployment scope: native search rewrites require BEs containing this reader change. Mixed-version/cloud smooth-upgrade compatibility with older search readers is outside this PR's scope; this PR does not add old-reader capability gating or rescheduling.
  • Existing explicit search TVF SQL-filter behavior is preserved. Vector replacements preserve the approximate functions' NOT NULL result contract through metric conversion, keeping existing Top-N and ancestor slot references consistent; explicit TVF result-column nullability is unchanged.
  • Bound MATCH query literals and physical search-column names preserve whitespace, including raw-tokenizer queries and quoted column names.
  • Approximate vector queries retain scalar execution when Lance field IDs remain unavailable after metadata enrichment, limit + offset exceeds the native UINT32 candidate budget, or the search TVF rejects native-query/schema validation. In particular, unused fields with reserved internal prefixes or case-insensitive duplicate names do not make an otherwise valid pruned vector scan fail. BM25 still reports unsupported native-search shapes explicitly. Native search limits do not restrict ordinary scalar SQL LIMIT/OFFSET.

Validation

  • Rebased onto branch-4.2 at 807c3a02a3. The FE Maven reactor build passed all 164 related tests, including 40 optimizer tests and the existing TVF, predicate conversion, scan planning, query encoding, prepared-search, and snapshot-selector suites. New whitespace, unavailable-field-ID, native-limit, vector-nullability, and unused-schema-validation regressions were observed failing before their respective fixes. Additional full-pipeline coverage checks the original L2/dot metrics on reserved-prefix and case-collision schemas, BM25 nullable scores with debug validation, and scalar fallback for secondary ordering/native vector validation. All five vector metrics/functions are covered through the full rewrite pipeline with fe_debug enabled and disabled, including ancestor projections; the external regression suite also executes vector queries with fe_debug=true.
  • Cosine/Hamming optimizer coverage includes metric mapping, ascending/descending order, result conversions, full rewrites with debug validation, real analyzer-inserted UInt8 widening, unsafe-cast fallback, invalid byte queries, zero/non-finite cosine norms, and exact-function scalar preservation. Added scalar SQL regressions and native-versus-TVF/scalar comparisons for cosine/Hamming (cluster execution pending CI).
  • FE Checkstyle passed.
  • New production cosine/Hamming kernel bodies passed boundary checks and 10,000 randomized Hamming vector-pair comparisons in a focused C++ harness. Changed BE production and test translation units passed C++ syntax checks; all affected C++ files passed clang-format 16 checks.
  • Dependency download/MD5 and Foyer patch lifecycle tests passed.
  • Reused the exact pinned lance-c revision rebuilt with the Foyer patch; native runtime smoke tests against that library passed for high-bit UINT64 row IDs, vector allowlists, empty allowlists, and prepared FTS allowlists with unchanged BM25 scores.
  • Added BE tests for query-local cache reuse, empty masks, the broad-filter fallback retaining the nearest row, and native NULL-vector/NaN-distance exclusion for both L2 and dot. Added an external-table regression suite comparing automatic SQL with explicit TVFs and scalar distance values.
  • Added a deterministic BE regression for initial stop, stop after a partial batch, stop at EOF before publication, and active-reader cache recovery/hits. A focused C++ harness compiling the production collector/cache method bodies with stubbed native/Arrow operations reproduced the stop bug before the fix and passed afterward, including RAII cleanup and retry assertions.
  • Extended the native reader cache test to verify retained memory accounting after reader closure, no duplicate charge on hits, and release on cache destruction (TLS accounting flushed before assertions). A focused C++ harness running the production collector/cache methods failed on untracked storage before the fix and passed afterward, including stop, broad-filter fallback, allocation-failure cleanup, and uncached retry.
  • Full BE unit-test execution and Lance external-table cluster regression execution remain for CI; they were not run locally. The scalar array-distance regression suite was executed locally using the CI-built FE/BE for d3e3f1b: the original suite reproduced the CI parser failure, and the corrected complete suite passed (1 suite, 0 failed, 0 fatal). Column-backed arrays now use array(number) rather than constant-only bracket literals. The temporary FE/BE were stopped after validation.

Release note

Support automatic native Lance search for eligible approximate vector Top-N and MATCH/BM25 SQL on Lance external tables.

Check List (For Author)

  • Test
    • Regression test added (execution pending CI)
    • Unit Test added (FE passed; BE execution pending CI)
    • Manual native API smoke tests
  • Behavior changed:
    • Yes. Eligible approximate vector queries use native candidate semantics (including the NULL/NaN differences above); eligible scored MATCH queries become supported.
  • Does this need documentation?
    • Yes. The SQL forms and current boundaries are described above; user documentation should accompany the feature release.

Check List (For Reviewer who merge this PR)

  • Confirm the release note
  • Confirm test cases
  • Confirm document
  • Add branch pick label

@Gabriel39
Gabriel39 requested a review from yiguolei as a code owner October 20, 2026 11:37
@hello-stephen

Copy link
Copy Markdown
Contributor

Thank you for your contribution to Apache Doris.
Don't know what should be done next? See How to process your PR.

Please clearly describe your PR:

  1. What problem was fixed (it's best to include specific error reporting information). How it was fixed.
  2. Which behaviors were modified. What was the previous behavior, what is it now, why was it modified, and what possible impacts might there be.
  3. What features were added. Why was this function added?
  4. Which code was refactored and why was this part of the code refactored?
  5. Which functions were optimized and what is the difference before and after the optimization?

@Gabriel39

Copy link
Copy Markdown
Contributor Author

run buildall

@Gabriel39

Copy link
Copy Markdown
Contributor Author

/review

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Static review of head 69268e3 found three P2 issues: lossy normalization of bound search values, loss of vector fallback when field IDs are unavailable, and applying the native UINT32 bound to valid scalar-fallback SQL. All are reported inline. No P0/P1 finding or existing blocking thread was identified.

The review converged after 3 rounds: all three reviewers returned NO_NEW_VALUABLE_FINDINGS on the unchanged final comment set. All 14 changed paths, candidates, and initial risk focuses have explicit dispositions. No additional user-provided focus was supplied.

Critical checkpoint conclusions:

  • Goal and proof: eligible approximate-vector Top-N and MATCH/BM25 SQL are lowered to pinned native search, but the three inline edge cases need fixes. New FE, BE and external regression tests were inspected; none was executed here.
  • Scope and clarity: changes are focused on the late rewrite, bound TVF construction, bounded row-ID prefilter, dependency API update, and tests.
  • Concurrency and locks: scanner workers share a scan-local cache; its shard mutex serializes complete factory publication. No new race, inverse lock order, or deadlock path was substantiated. The source probes at most 65,537 survivor IDs; runtime contention was not measured.
  • Lifecycle and initialization: Arrow readers, native scanners, copied masks, callback contexts, and dataset snapshots have consistent ownership on inspected exits. No new ownership cycle or cross-translation-unit static initializer was introduced.
  • Configuration: no new configuration or dynamic-update behavior.
  • Compatibility: no changed persisted format; existing search-request representation is reused. The pinned native archive checksum matches and the Foyer patch applies in a static dry-run. Runtime rolling-upgrade compatibility was not tested.
  • Thrift/protobuf: no definitions changed, so cross-release field/enum comparison is inapplicable.
  • Parallel paths: normal scans, exact/scalar and approximate vector paths, native indexed/flat search, prepared FTS, and explicit TVF SQL filters were traced. The two vector fallback regressions and shared normalization issue are reported inline.
  • Special conditions: direct-scan/single-key, fixed snapshot, supported Float32 shape, nonmovable projection, exact-predicate conversion, monotone score bounds, and broad/empty mask conditions were checked. The native-bound guard and unavailable-ID handling need the reported fallback corrections.
  • Test coverage: new cases cover metric conversions, global OFFSET, snapshot binding, output identity/pruning, MATCH variants, score bounds, nonmovable expressions, cache reuse, empty masks, broad-filter fallback and intended native NULL/NaN semantics. The three reported FE edge cases lack focused regression coverage.
  • Test results: no generated output file changed. Assertions were read statically; no passing runtime result is claimed.
  • Observability: new prefilter time/row/cache/fallback counters and retained native statistics callbacks cover the new mechanism; no further concrete omission was found.
  • Transactions and persistence: read-only query changes; no EditLog, commit/publish or failover protocol modification. Snapshot/version and schema binding were traced.
  • Data writes and crash atomicity: no write path changed. Reader cancellation/error cleanup was inspected; crash behavior was not exercised.
  • FE-BE values: query/filter/prefilter state uses existing request fields and matching reader paths; no new separately transmitted variable was found missing.
  • Performance: bounded per-scan ID caching, full-filter fallback, and column pruning are preserved. No additional concrete hot-path regression was substantiated; no benchmark was run.
  • Other issues: final full-scope sweeps found no further verified candidate. Initial predicate/identity/snapshot/lifetime risks were either covered by the inline findings or dismissed with source evidence.

Validation limits: static analysis only; no Doris builds, unit-test runs, cluster regressions, runtime concurrency/cancellation checks, or performance experiments. Exhaustive external-writer/type/predicate compatibility was not demonstrated. These limits do not leave an unresolved code candidate in the reviewed scope.

Review converged after 3 round(s).

@hello-stephen

Copy link
Copy Markdown
Contributor

FE UT Coverage Report

Increment line coverage 75.51% (148/196) 🎉
Increment coverage report
Complete coverage report

@hello-stephen

Copy link
Copy Markdown
Contributor

BE Regression && UT Coverage Report

Increment line coverage 83.46% (111/133) 🎉

Increment coverage report
Complete coverage report

Category Coverage
Function Coverage 76.42% (34430/45055)
Line Coverage 61.24% (388910/635032)
Region Coverage 58.22% (328559/564327)
Branch Coverage 59.13% (151321/255914)

@hello-stephen

Copy link
Copy Markdown
Contributor

FE Regression Coverage Report

Increment line coverage 72.45% (142/196) 🎉
Increment coverage report
Complete coverage report

yiguolei
yiguolei previously approved these changes Oct 21, 2026
@Gabriel39
Gabriel39 force-pushed the dev/lance-search-expression-pushdown-4.2 branch from 69268e3 to 16f8c09 Compare October 21, 2026 11:15
@Gabriel39

Copy link
Copy Markdown
Contributor Author

run buildall

@Gabriel39

Copy link
Copy Markdown
Contributor Author

/review

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Static review of PR #69346 at 16f8c09 found one new P2: automatic vector rewriting widens the projected distance's nullability while retaining the original Top-N reference, causing final nullable validation to reject eligible L2/dot queries when fe_debug=true (including fuzzy mode). See the inline plan and control-flow evidence. No new P0/P1 was confirmed.

The three existing P2 threads are fixed on this head: bound physical names/query whitespace are preserved; unavailable Lance field IDs retain scalar vector execution; and an unrepresentable native limit/offset budget retains scalar vector execution. They are duplicate fences and are not reposted. There are no existing P0/P1 blockers to carry forward. The supplied focus file contains no additional user concerns.

Critical checkpoint conclusions:

  1. Goal and proof: the direct-scan SQL-to-search path implements approximate L2/dot and MATCH/BM25 Top-N by reusing fixed-snapshot search planning, global merge, and deferred payload reads. The nullable-validation defect limits the vector goal in debug/fuzzy mode. FE unit, BE unit, and external regression assertions were inspected; none were executed in this static-only review.
  2. Scope: all 12 changed paths were reviewed. The change is focused on the rewrite, bound TVF factories, bounded row-ID prefilter, registration, and tests.
  3. Concurrency and locks: scanner workers use the existing task context. Completed query-local cache values are published under shard mutexes; first-fill I/O runs under that lock. No reachable recursive acquisition or conflicting lock order was found.
  4. Lifecycle/static initialization: auxiliary scanner/Arrow reader and main scanner/context/dataset ownership use existing RAII and close order. No new cross-TU static dependency or reference cycle was found.
  5. Configuration: no configuration item was added. The existing fe_debug/fuzzy-mode interaction is the inline issue.
  6. Compatibility: existing SQL TVF normalization/filter timing remains intact; bound-scan factories preserve SQL data. Snapshot/branch URI/version binding is retained. The existing SUBSTRAIT/pre-filter protocol fields and pinned native API are reused; no persisted format change was introduced.
  7. Thrift/protobuf: no definitions changed; no new field/enum IDs require cross-branch comparison.
  8. Parallel paths: exact vector functions, reverse ordering, incompatible vector schemas, missing field IDs, unsupported predicates, and excessive vector budgets retain scalar paths. JOIN/EXISTS and secondary ordering keys are outside this direct-scan rule. Explicit TVF and normal scan paths were checked.
  9. Conditional checks: native candidate budget, index enrichment, projection mobility, and lower-score-bound eligibility were traced. Global OFFSET remains above per-split limit+offset candidate retrieval; unsafe score upper bounds fail explicitly.
  10. Test coverage: inspected metric conversions, filter-before-ranking, snapshot enrichment, scalar fallbacks, whitespace, score bounds, cache reuse, empty masks, broad fallback, and documented native NULL/NaN exclusion. The missing full-pipeline debug/fuzzy-mode nullability test exposes the finding. Concurrency/cancellation and combined branch/stable-ID/index-segment/payload scenarios remain runtime validation opportunities.
  11. Test results: no golden result files changed; expected values and comparison/tolerance assertions in the changed tests were inspected.
  12. Observability: new collection time, candidate count, cache-hit, and broad-fallback counters supplement existing native scan statistics. No additional correctness issue was confirmed.
  13. Persistence/failover: no transaction or EditLog mutation changed; scans use a pinned immutable dataset snapshot.
  14. Data writes/atomicity: no write path changed. Cancellation, errors, EOF, and reader cleanup were inspected.
  15. FE-to-BE transfer: validated binary filters reuse the existing request fields and reach both vector and full-text readers; SQL-only embedded-NUL validation remains separate. No new transmitted variable requires additional send paths.
  16. Performance/memory: row-ID collection admits at most 65536 IDs, shares completed values per scan, and discards partial broad lists before retaining the full native filter. No independent performance defect was proven; runtime cost and distributed behavior were not benchmarked.
  17. Other invariants: no committed-data/version/delete-bitmap mutation is introduced. Source slot IDs, output pruning, cache disabling, native row identity, pre-ranking masks, and prepared full-text score context were traced. The nullability inconsistency is the sole accepted new finding.

The pinned native implementation confirms copied _rowid allowlists, stable-ID/address compatibility, empty-mask semantics and preserved prepared full-text scorers: lance-c scanner, prepared FTS.

Review converged in two rounds after all three reviewers rechecked the frozen finding set and returned NO_NEW_VALUABLE_FINDINGS. No unresolved source-level candidates or uncovered changed paths remain. Validation is static only; builds, tests, cluster regression, runtime performance, and the full execution cross-product were not run under this contract.

Review converged after 2 round(s).

@hello-stephen

Copy link
Copy Markdown
Contributor

BE Regression && UT Coverage Report

Increment line coverage 83.46% (111/133) 🎉

Increment coverage report
Complete coverage report

Category Coverage
Function Coverage 76.02% (34316/45142)
Line Coverage 60.79% (386743/636197)
Region Coverage 57.61% (325646/565297)
Branch Coverage 58.55% (150212/256574)

@hello-stephen

Copy link
Copy Markdown
Contributor

FE Regression Coverage Report

Increment line coverage 71.15% (148/208) 🎉
Increment coverage report
Complete coverage report

@Gabriel39

Copy link
Copy Markdown
Contributor Author

run buildall

@Gabriel39

Copy link
Copy Markdown
Contributor Author

/review

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One new P2 finding is reported inline: an unrelated, unprojected column with Doris's internal global-rowid prefix makes the automatic vector rewrite fail TVF schema validation instead of preserving the previously valid scalar plan. Both approximate vector metrics are affected. No new P0/P1 issue was verified.

The four existing P2 findings were independently revalidated as fixed at this head: bound query/column whitespace, missing field-ID fallback, native LIMIT/OFFSET overflow fallback, and vector-result nullability including ancestor references with fe_debug. None is reposted; no existing P0/P1 blocker remains to carry forward. The focus file contains no additional user concerns.

All 12 changed paths and their relevant callers, downstream execution, tests, and parallel paths were reviewed. The following are the required critical-checkpoint conclusions:

Checkpoint Conclusion
Goal and behavior Eligible approximate L2/dot and MATCH/BM25 Top-N reuse fixed-snapshot native search; metric conversion, global OFFSET and prefiltering are preserved. The inline schema-eligibility defect remains.
Scope Changes are focused on one registered rule, bound-search factories, bounded BE prefiltering, and tests. Existing search planning is reused.
Concurrency and locks Cache creation/publication uses the existing shard mutex; published lists are immutable. Native prefilter creation holds that lock once for the shared key. No recursive acquisition, new lock order, or new thread was found.
Lifecycle and static initialization Same pinned dataset supplies IDs and search. Native setters copy IDs; Arrow/scanner RAII covers EOF, broad fallback, errors and cancellation. No new static initialization dependency or ownership cycle was found.
Configuration No configuration or session variable is added. Regression changes to fe_debug are restored.
Compatibility No persisted format or Doris function ABI changes. Existing SQL-TVF requests remain supported; the native allowlist API is already in the base dependency. The upgrade assessment follows the documented BE-before-FE sequence; no supported rolling-upgrade defect was established.
Thrift/protobuf No definitions or IDs change; cross-release ID comparison is inapplicable. Existing request fields carry the binary Substrait payload.
Parallel paths L2/dot, MATCH_ANY/ALL/PHRASE, indexed/unindexed vectors, normal scans, explicit TVFs and scalar fallbacks were checked. Joins/EXISTS and secondary order keys are outside the rewrite.
Conditional guards Direction, native budget, Float32 fixed-size vectors, complete predicate pushdown, field IDs and non-movable projections have concrete safety guards. The inline finding identifies a missing schema guard.
Test coverage Inspected FE full-pipeline/nullability/ExprId, bound snapshot, whitespace and fallback cases; BE cache, empty-mask, broad-filter, FTS-score and NULL/NaN cases; and SQL comparisons with explicit TVFs/scalar distances. The reserved-prefix fallback case is missing.
Expected results No golden output files change. Assertions and metric conversions were inspected; FTS ties are compared without assuming physical order.
Observability Added prefilter time/row/cache-hit/fallback counters cover the new path; native statistics use the existing nonthrowing callback boundary.
Transactions and persistence Read-only snapshot optimization; no EditLog, recovery, transaction visibility or MoW delete-bitmap changes.
Data writes and atomicity No data writes. A partial survivor list is never published; broad filters retain the full native predicate and empty lists select zero rows.
FE-to-BE propagation Both search kinds receive prefilter=true and the existing search-filter field; global Top-N retains the offset while each split fetches limit+offset candidates. No point-query/constant-fold variable is introduced.
Performance and memory Row-ID-only prefiltering avoids payload reads, shares a bounded 65,536-ID list per scan, and preserves column pruning/deferred reads. Broad filters incur an additional scan before native fallback. No additional verified performance defect was found.
Further recheck Native stable-row-ID space, branches, cache identity, filter composition, float ordering, score lower bounds, slot lifetimes and unusual schemas were rechecked. Only the inline P2 is accepted.

Validation was static only: no builds, tests, source modifications, or submodule initialization were performed. Runtime and cluster behavior remain unverified by this review; authored tests were inspected rather than executed.

Review converged after 2 rounds: all three assigned reviewers returned NO_NEW_VALUABLE_FINDINGS on the same frozen inline set. Every risk/candidate has an evidence-backed disposition; no unresolved static candidate or uncovered source scope remains. Runtime validation is outside this static-only review.

Review converged after 2 round(s).

@Gabriel39

Copy link
Copy Markdown
Contributor Author

run buildall

@Gabriel39

Copy link
Copy Markdown
Contributor Author

/review

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed exact head 53ce165cf7cc99b755234daf18900b1bf3006404 across all 12 changed paths. One new P2 finding: an automatic vector rewrite with a pushable WHERE predicate can send a Substrait search request to an incompatible old BE during cloud smooth upgrade, breaking a query that previously used scalar execution. No new P0/P1 finding was confirmed.

The five supplied P2 threads were independently revalidated as fixed: bound whitespace, unavailable field IDs, native LIMIT/OFFSET overflow, vector result nullability, and unrelated reserved-prefix/schema validation. They were not reposted. No already-reported P0/P1 finding remains to carry forward. The focus file supplied no additional concerns.

Critical checkpoint conclusions:

  1. Goal: direct Lance approximate L2/dot Top-N and MATCH/BM25 rewrite into existing fixed-snapshot search relations; source and test definitions support the intended scope, subject to the compatibility finding.
  2. Scope: the late rewrite, bound-scan factories, BE prefilter and tests remain focused and reuse existing search planning/execution.
  3. Concurrency: the cache is local to a scan operator; a shard mutex serializes creation/publication and cached results are immutable. Same-key scanner work is intentionally shared. No new threads or lock-order cycle was found.
  4. Lifecycle: scanner/Arrow ownership, split reset, EOF, cancellation and error cleanup were traced. No new reference cycle or cross-TU static initialization dependency was found.
  5. Configuration: no new configuration was added; the extra scanner uses existing scan options.
  6. Compatibility: FE-1 identifies the missing old-reader capability boundary. Existing explicit-TVF SQL filters retain their path. Native approximate NULL/NaN eligibility differences are deliberate; exact functions remain scalar.
  7. Definitions: no Thrift/protobuf definitions, field IDs, enum values or storage formats change.
  8. Parallel paths: both vector metrics and vector/FTS filtering were checked, including indexed/unindexed splits and broad native-filter fallback; unsupported shapes retain their documented fallback/error behavior.
  9. Conditions: candidate-budget, schema, field-ID, non-movable expression and predicate-completeness guards were traced. Native row-ID inputs match the pinned lance-c contract, including stable IDs. Branch identity is encoded in the dataset URI.
  10. Tests: FE direct/full-rewrite tests, BE filter/cache/empty/broad/NULL/NaN tests and external TVF/scalar comparisons were inspected, including negative/fallback cases. Runtime validation was not performed.
  11. Results: no expected-output files change; new assertions are consistent with the inspected fixture and planning contracts. No passing test result is claimed.
  12. Observability: prefilter elapsed time, IDs, cache hits and fallbacks are exposed. A broad prefilter stopped before Arrow EOF may omit its native I/O/index statistics; elapsed/fallback counters remain available. This narrow attribution limit was not accepted as an inline defect.
  13. Persistence: no EditLog, commit, replay, master-failover or partition-visibility path changes.
  14. Writes: the change is read-only; no transaction/data-write atomicity changes. Partial prefilter lists are discarded rather than published as complete results.
  15. FE-BE propagation: both factories use the shared request/scan serialization. Local candidates use LIMIT+OFFSET with zero local offset, and the retained Doris Top-N applies the final global OFFSET.
  16. Performance: row-ID-only projection, a 65,536-ID cap, cache reuse and output pruning bound the added C++ list and avoid reintroducing wide payload columns. No additional material performance defect was confirmed statically.
  17. Other invariants: source ExprIds/nullability, snapshot/version/branch selection, prepared FTS scoring, deleted-row handling and SQL-cache disabling were checked; no further actionable defect was confirmed.

Source review coverage is complete. Validation limits remain: no builds or tests were run under the static-only contract, so full BE/cluster execution and injected concurrency/cancellation/error cases remain unverified. No unresolved source candidate or risk focus remains.

Converged after 2 rounds: all three latest-round reviewers returned NO_NEW_VALUABLE_FINDINGS on the unchanged comment set and anchor; all changed paths, candidate dispositions and risk focus items are complete.

Review converged after 2 round(s).

@Gabriel39

Copy link
Copy Markdown
Contributor Author

run buildall

@Gabriel39

Copy link
Copy Markdown
Contributor Author

/review

@Gabriel39

Copy link
Copy Markdown
Contributor Author

Local pipeline review — ✅ PASS

schema: doris-repo-review/v1
status: PASS
pr: apache/doris#69346
commit: ef963fb13ab15d7aace56aec666c26d3f1aa49f7
base: 807c3a02a37d667cc632b6d5096ce2e1e22b8b3b
reviewed_at: 2026-10-22T11:06+08:00
reviewer: Gabriel39
model: gpt-6-astra
effort: xhigh
findings: {blocker: 0, major: 0, minor: 0, nit: 0}
rounds: 1
converged: true

Notes for maintainers

  • Scope requires BEs containing this reader change. Mixed-version/cloud smooth-upgrade compatibility with older readers was explicitly excluded; old-reader capability gating and rescheduling are not implemented.
  • Static review only: no builds, tests or cluster operations were run during this review. Full BE unit tests and cluster regression execution remain for CI.

Reviewed locally with the doris-repo-review pipeline. Repository policy may accept this receipt for the matching commit; it is not a human Apache approval.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed all 12 changed paths and their FE-to-BE search flow at ef963fb. One new P2 is reported inline: the cached row-ID vector retains query data without charging its capacity to the query memory tracker. No new P0/P1 was verified.

Existing-thread dispositions: the missing-field-ID fallback issue in discussion_r4324652892 still applies. The real LanceCatalogClient.loadTableMetadataForSearch rejects FIELD_IDS_UNAVAILABLE before the rule reaches its new fallback guard; the added test mocks that API to return the unavailable state. This is the same reported failure and is not reposted. The reserved-prefix/schema-validation fallback, vector nullability, UINT32-limit fallback, and bound whitespace fixes were independently confirmed in code. The old-reader SUBSTRAIT incompatibility in discussion_r4343983922 remains acknowledged and explicitly excluded from the PR's requested scope. No existing P0/P1 inline finding remains to carry into the blocking-ID list.

Critical-checkpoint conclusions:

  1. Goal and correctness: the rule selects native approximate L2/dot or MATCH/score search using a fixed snapshot, complete ordinary predicates, per-split limit+offset candidates, and the retained global Top-N. The new accounting issue and known field-ID fallback failure need attention.
  2. Scope and reuse: the changes are focused and reuse existing search TVFs, native scanner APIs, and scan-local caching.
  3. Concurrency and locks: concurrent scanner tasks use the existing shard mutex for single-flight immutable cache publication. Native fill runs under that mutex; no nested cache acquisition, reverse lock order, or concrete deadlock was found. No new thread is introduced.
  4. Lifecycle and initialization: temporary scanners, Arrow readers, and batches have RAII ownership; interruption/error avoids cache publication. Cache entries end with FileScanLocalState. No new ownership cycle or static initialization dependency was found.
  5. Configuration: no configuration item is added; existing read-ahead and session controls are reused.
  6. Compatibility: storage formats and API definitions are unchanged. Current BEs accept the new SUBSTRAIT use of the existing search carrier; the acknowledged old-reader limitation above remains.
  7. Shared definitions: no gensrc/thrift or gensrc/proto file changes, so cross-release field-ID/enum comparison is inapplicable.
  8. Parallel paths: L2/dot, MATCH_ANY/ALL/PHRASE, normal scans, explicit TVF SQL filters, automatic Substrait filters, indexed/unindexed splits, and empty/broad prefilter paths were traced.
  9. Conditions and fallback: direction, secondary-sort, vector physical type, native candidate budget, schema/name validation, and complete-filter checks are explained and conservative. The strict field-ID enrichment boundary is the existing exception noted above.
  10. Tests: inspected FE full-rewrite/negative cases, BE cache/empty/broad/stop/score cases, and external SQL comparisons. Missing real-client field-ID coverage is material; concurrent fills, injected native errors, stable-ID writer fixtures, broad FTS execution, and memory-byte accounting have test-depth limits. No build or test was run under this static-only contract.
  11. Expected results: no golden output file changes. Inspected row, score, metric, nullability, and ExprId assertions agree with the intended contracts; execution is unverified.
  12. Observability: EXPLAIN retains kind/metric/version/branch; new prefilter timers, survivor counts, hits, and fallbacks complement existing native IO/index counters.
  13. Persistence and failover: no transaction, EditLog, replay, or persisted metadata mutation is added.
  14. Writes and atomicity: the feature is read-only; write/crash-atomicity behavior is unchanged.
  15. FE-BE propagation: the bound request uses the existing external-search carrier and current BE validation/dispatch handles SUBSTRAIT. No new transmitted variable or alternate RPC sender is introduced.
  16. Performance and memory: required-output pruning avoids reintroducing embeddings and payloads; bounded shared prefiltering avoids repeated full survivor collection across splits. The inline finding covers retained untracked C++ capacity. No additional substantiated performance defect was found.
  17. Other invariants and remaining limits: external snapshot/version and branch identities remain pinned, native allowlists use dataset _rowid space including stable IDs, and prepared FTS retains global scoring. These native semantics were verified against the pinned lance-c API and its implementation. Doris partition-version, MoW and delete-bitmap mutations are not involved. There was no additional user-provided focus. Runtime/native integration remains unexecuted; no changed path or review candidate remains unclassified.

Converged after 2 review rounds: all three latest reviewers returned NO_NEW_VALUABLE_FINDINGS on the unchanged one-comment set; all changed paths, risk focuses, and candidates have explicit dispositions. Static review execution limits are recorded above.

Review converged after 2 round(s).

Comment thread be/src/format_v2/table/lance_reader.h Outdated
struct RowIdPrefilter {
Status status;
// Empty selects no rows; nullopt keeps the provider's native filter for a broad result.
std::optional<std::vector<uint64_t>> row_ids;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Track the cached row-ID buffer against query memory

A supported prefilter matching 65,536 rows retains at least 512 KiB in this plain std::vector: _read_row_id_prefilter() moves the accumulated IDs into RowIdPrefilter, and ShardedKVCache keeps them until FileScanLocalState is destroyed. Standard allocations on this branch only pass through the jemalloc hook, whereas DorisVector uses the allocator that charges the query tracker. These retained buffers therefore go unaccounted across parallel scan instances, so query memory reporting and limit enforcement miss their capacity. Use tracked ownership for both the accumulation buffer and the cached IDs, preserving the existing survivor bound.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in d3e3f1b. Both the in-flight collector and the query-local cached allowlist now use DorisVector<uint64_t>, so buffer allocations participate in query memory checks and accounting. The reader regression now verifies retained accounting after reader closure, unchanged consumption on cache hits, and release on cache destruction, flushing TLS accounting before assertions. A focused harness executing the production collector/cache methods also verifies cleanup on stop, broad-filter fallback, and allocation failure, followed by an uncached retry.

@Gabriel39

Copy link
Copy Markdown
Contributor Author

run buildall

@Gabriel39

Copy link
Copy Markdown
Contributor Author

/review

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Static review of f5b14c4: no new, nonduplicate actionable finding was confirmed. The main review and three independent reviewers covered all 22 changed paths, related callers and consumers, and the supplied skill checkpoints. The review converged in one round with an empty new inline set. No unresolved review candidate or uncovered changed path remains.

Existing context remains relevant: P2 row-ID memory accounting still applies to the accumulating and cached std::vector buffers. P2 old-reader compatibility remains an acknowledged deployment limitation: older search readers reject the generated SUBSTRAIT filter, and mixed-version/cloud smooth-upgrade support is explicitly outside the PR's scope. Neither issue is reposted. The earlier schema-validation fallback, result-nullability, native-limit, unavailable-field-ID, and bound-whitespace threads have corresponding fixes and regression coverage in this head. There are no existing P0/P1 comments to carry as blockers.

The reviewed flow retains Doris's global TopN over a bound native search relation, requests limit+offset candidates per split with native offset zero, applies complete ordinary predicates before candidate pruning, and keeps only monotone score lower bounds as residuals. Snapshot metadata, projected source ExprIds, result nullability, and column pruning survive the rewrite and physical translation. Native allowlists consume the same dataset's _rowid domain, copy their input, and treat an empty list as no rows; broad masks discard the entire partial list and retain native filtering.

Critical checkpoint conclusions:

  1. Goal and proof: supported approximate vector and MATCH/BM25 shapes use the existing fixed-snapshot search path; FE, BE and SQL tests cover the intended behavior in source. Runtime success was not verified here.
  2. Scope and reuse: the rewrite, bound factories, reader prefilter and scalar additions are focused and reuse existing planning/execution and distance templates.
  3. Concurrency and locks: scanner workers share a scan-local cache; shard locking provides one mask construction per immutable key, with no nested cache acquisition or new lock-order cycle found.
  4. Lifecycle and static initialization: temporary scanner/Arrow stream/value ownership is RAII; stop/error results are never published; no new cross-translation-unit static initialization dependency.
  5. Configuration: no new item or dynamic-propagation requirement; existing scan options are reused.
  6. Compatibility: no persisted format change; matching FE/BE function registrations are present. Old-reader compatibility remains the existing limitation described above.
  7. Thrift/proto: no definitions changed, so release-branch field/enum allocation checks are inapplicable.
  8. Parallel paths: normal scans, explicit TVF SQL filters, native vector/FTS searches and all five approximate scalar fallbacks were checked. Exact functions retain scalar execution.
  9. Conditional checks: direct-scan/single-key/order/type guards, non-movable projection rejection, native-budget checks, metadata enrichment and optional-vector fallback prevent unsupported substitutions.
  10. Test coverage: reviewed metric/value/order/offset, snapshot/slot/pruning, invalid schema/query/predicate, cosine norm, Hamming coercion/bytes, empty/broad/stopped masks, cache recovery and FTS-score assertions. Full BE and cluster execution remain CI work.
  11. Expected results: no output golden files changed; new assertions align with the declared scalar/native metric and candidate contracts on inspection.
  12. Observability: prefilter time, rows, cache hits and fallback counters supplement existing scan statistics and explain information; no new per-row logging.
  13. Persistence and failover: read-only change, no EditLog or transaction mutation; bound version/URI/schema checks preserve snapshot identity.
  14. Writes and crash cleanup: no data-write atomicity path changed; temporary resources and scan-local cache ownership have explicit cleanup.
  15. FE-BE passing: existing search requests carry metrics, query values, candidate bounds, prefilter and SUBSTRAIT payload through split serialization and reader validation/application.
  16. Performance and memory: pruning avoids reintroducing unused payload/vector columns; row-ID collection is projected and bounded at 65,536 IDs, with shared reuse and complete-filter fallback. The existing memory-accounting defect remains as noted.
  17. Additional correctness recheck: inspected late TVF slot copying, score residual monotonicity, const/nullable arrays, Hamming pre-coercion/range validation, native ID ownership, EOF publication and scanner-local cancellation. Documented approximate NULL/NaN eligibility and unsupported stored inner-NULL elements are intentional contract boundaries.

The user focus file contains no additional concerns. Validation was static only: no builds, tests, source modifications or submodule initialization were performed. Convergence describes review coverage and candidate disposition; it does not claim executed BE or cluster validation.

Review converged after 1 round(s).

@Gabriel39

Copy link
Copy Markdown
Contributor Author

run buildall

@Gabriel39

Copy link
Copy Markdown
Contributor Author

/review

@Gabriel39

Copy link
Copy Markdown
Contributor Author

Local pipeline review — ✅ PASS

schema: doris-repo-review/v1
status: PASS
pr: apache/doris#69346
commit: d3e3f1b0015a8bfd7c99f88c47d287dde7096101
base: 807c3a02a37d667cc632b6d5096ce2e1e22b8b3b
reviewed_at: 2026-10-22T13:45+08:00
reviewer: Gabriel39
model: gpt-6-astra
effort: xhigh
findings: {blocker: 0, major: 0, minor: 0, nit: 0}
rounds: 1
converged: true

Notes for maintainers

  • The row-ID memory-accounting issue is fixed: both collection and cache retention use the query-tracked allocator; scanner stop and complete-only cache publication remain intact.
  • This is a static review of the complete PR, carrying independently verified unchanged code. Full BE unit tests and cluster regressions remain pending CI; supplied syntax checks and focused harness results are not a full BE test run.
  • Old-BE rolling/cloud smooth-upgrade compatibility is explicitly outside the requested scope. Deployment requires BEs containing the reader changes; this PASS does not certify mixed-version compatibility.

Reviewed locally with the doris-repo-review pipeline. Repository policy may accept this receipt for the matching commit; it is not a human Apache approval.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Static review of d3e3f1b: no new actionable findings confirmed. Main review covered the complete supplied diff and all 22 changed paths; the FE, BE and separate risk reviewers each returned NO_NEW_VALUABLE_FINDINGS in round 1. All candidates and risk questions have evidence-backed dispositions. Review converged in one round.

Critical checkpoint conclusions:

  1. Goal and evidence: the five approximate vector functions and eligible MATCH/BM25 Top-N queries reuse fixed-snapshot search planning. FE rewrite tests, BE reader/kernel tests and SQL differential regressions cover positive and negative paths; none were executed during this static-only review.
  2. Scope: the change is focused on scalar registration/kernels, the optional optimizer rule, bound search factories and bounded native prefiltering; existing planner and reader interfaces are reused.
  3. Concurrency: scanner tasks share immutable cached survivors through the existing shard mutex. Creation coalesces identical requests; no new thread entry, nested lock cycle or mutable shared-buffer race was found.
  4. Lifecycle: scanner/Arrow resources and tracked collection buffers use RAII. Interrupted or failed collections are not published; cache ownership outlives readers, with explicit stop checks and broad-filter fallback. No new static initialization dependency was found.
  5. Configuration: no new production configuration is introduced; existing native scan options apply to the collector.
  6. Compatibility: no storage format or existing scalar signature changes. Current search readers are required. The acknowledged old-reader/cloud-upgrade limitation remains in existing thread r4343983922; it is not reposted or claimed fixed.
  7. Shared definitions: no Thrift/protobuf definitions changed, so cross-release ID comparison is inapplicable.
  8. Parallel paths: all five metric mappings and BM25 operators were traced. Exact functions, unsafe casts/types, unsupported predicates, reverse/secondary ordering and native-budget overflow preserve scalar execution or the documented explicit BM25 error. Normal scans and explicit TVF SQL filtering retain their paths.
  9. Conditional checks: complete predicate conversion, physical Float32/UInt8 gates, cosine norm validation, fixed-version enrichment, metadata identity and empty-versus-broad allowlist handling have concrete safety purposes. Native NULL/NaN candidate behavior and unsupported inner-NULL elements are expressly documented domain boundaries.
  10. Test coverage: inspected full-pipeline ExprId/nullability checks, metric conversions, invalid queries/coercions, schema validation, score thresholds, selective/empty/broad masks, stopped-cache recovery and memory retention/release tests. Runtime, concurrency stress and performance measurements remain unverified.
  11. Expected results: added tests use direct assertions and expected-error blocks; no golden output files changed. No author-reported execution result is presented as this review's validation.
  12. Observability: existing search Explain/profile information plus prefilter time, survivor, hit and fallback counters cover the new path without per-row INFO logging.
  13. Persistence and failover: no EditLog, transaction commit/replay or persistent metadata write path changes. Bound snapshot version/branch identity is retained through search split planning.
  14. Writes and atomicity: the change is read-only planning/scanning and stateless scalar evaluation; no data-write atomicity protocol changes.
  15. FE/BE transport: existing search request fields carry SUBSTRAIT and prefilter state, with corresponding BE validation and setup. Scalar names, argument types and FLOAT/nullability contracts agree across FE and BE.
  16. Performance and memory: column pruning is retained, native splits request limit+offset with offset zero, and Doris applies the global Top-N/OFFSET. Row-ID buffers use DorisVector, are survivor-bounded and shared within a scan; broad predicates retain the full native filter. The pinned native API confirms the allowlist accepts dataset _rowid values and copies its inputs.
  17. Final recheck: no additional supported-domain defect was confirmed in output-slot copying, deferred materialization, score-bound monotonicity, native identity, cancellation/error publication or scalar const/nullable handling.

Existing threads were treated as duplicate fences. The memory-accounting, whitespace, unavailable-field-ID, native-limit, schema-validation and nullability fixes are present at this head; the old-reader limitation remains acknowledged. No existing P0/P1 finding requires carry-forward. There was no additional user-provided focus. All required static scope is covered; builds, tests, submodule initialization and source edits were excluded by the review contract.

Review converged after 1 round(s).

@hello-stephen

Copy link
Copy Markdown
Contributor

FE UT Coverage Report

Increment line coverage 76.33% (229/300) 🎉
Increment coverage report
Complete coverage report

@hello-stephen

Copy link
Copy Markdown
Contributor

FE Regression Coverage Report

Increment line coverage 70.57% (223/316) 🎉
Increment coverage report
Complete coverage report

@hello-stephen

Copy link
Copy Markdown
Contributor

BE UT Coverage Report

Increment line coverage 87.36% (152/174) 🎉

Increment coverage report
Complete coverage report

Category Coverage
Function Coverage 64.27% (29901/46522)
Line Coverage 49.12% (315443/642162)
Region Coverage 44.97% (254117/565090)
Branch Coverage 46.61% (119928/257326)

@hello-stephen

Copy link
Copy Markdown
Contributor

BE Regression && UT Coverage Report

Increment line coverage 87.36% (152/174) 🎉

Increment coverage report
Complete coverage report

Category Coverage
Function Coverage 76.37% (34503/45181)
Line Coverage 61.18% (389551/636781)
Region Coverage 58.01% (328352/565988)
Branch Coverage 58.97% (151563/257027)

@Gabriel39

Copy link
Copy Markdown
Contributor Author

run buildall

@Gabriel39

Copy link
Copy Markdown
Contributor Author

/review

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No new actionable finding was verified at head 4ef2fbb. Static review covered all 22 changed paths and the relevant planner, snapshot, split, native-reader and scalar-function interactions. The review converged in round 1: both complete-scope reviewers and the separate risk reviewer returned NO_NEW_VALUABLE_FINDINGS on the same unchanged empty finding set; every considered candidate and risk has an evidence-based disposition.

Critical checkpoint conclusions:

  • Goal and test evidence: the direct-scan/single-key rewrite routes eligible approximate L2, dot, cosine and Hamming Top-N and MATCH/BM25 through existing pinned-snapshot search. Added FE, BE and SQL tests represent the intended behavior; this review did not execute them.
  • Scope and reuse: changes are focused on the rewrite, bound-search factories, prefilter reader and related scalar registrations/tests; existing TVF, split-planning and distance infrastructure is reused.
  • Concurrency and locking: existing scanner workers share the scan-local immutable completed allowlist through the existing shard mutex. Native I/O occurs in the serialized cache creator; no new lock cycle, publication race or thread-entry tracking requirement was identified.
  • Lifecycle and static initialization: Arrow stream/reader, scanner, dataset and cache ownership were traced through success, EOF, error, cancellation and local early stop. Interrupted collectors are not cached. No new cross-translation-unit static-initialization dependency was introduced.
  • Configuration: no configuration item or dynamic-update propagation was added.
  • Compatibility: ordinary scans and explicit TVF SQL filters retain their paths; exact functions remain scalar. The already-reported P2 old-reader SUBSTRAIT incompatibility remains applicable and explicitly acknowledged as a mixed-version/cloud smooth-upgrade deployment limitation. It is not fixed or reposted here.
  • Shared definitions: no Thrift/protobuf definition, field ID, enum value or stored format changed, so release-branch ID comparison is not applicable.
  • Parallel paths: all five approximate vector functions, vector versus prepared FTS, indexed versus flat/uncovered splits, and scalar fallbacks were checked. Global OFFSET stays in Doris; each search split retains limit+offset candidates with offset zero.
  • Conditional guards: single-key/natural-order, physical Float32/UInt8, safe casts, query norm, native candidate bounds, complete predicate conversion, field IDs and TVF validation prevent unsupported vector rewrites. Only monotonic score lower bounds remain residual; unsupported BM25 shapes fail explicitly.
  • Test coverage: inspected positive and negative cases for metrics/conversions, snapshot pinning, debug nullability, schema/name validation, unsupported predicates, limits, byte/type/dimension/norm checks, selective/empty/broad allowlists, stopped producers/retry, memory retention/release and preserved global BM25 scores. No uncovered changed path or unresolved concrete candidate remains.
  • Expected results: no checked-in result file changed. Added assertions and fixture ordering were inspected statically, including Hamming bit counts, scalar cosine zero/empty behavior and native value conversions.
  • Observability: existing search EXPLAIN/profile fields plus new prefilter time, survivor count, cache-hit and fallback counters expose the affected behavior; statistics callbacks contain exceptions.
  • Transactions and persistence: these read-only query changes do not alter committed-data visibility, visible-version selection, MoW delete bitmaps, EditLog/replay or failover protocols. Search and prefilter use the same fixed dataset snapshot.
  • Writes and crash atomicity: no data-write or persistent mutation path was added; temporary reader resources unwind through existing RAII ownership.
  • FE/BE values and types: existing search-request serialization carries the complete SUBSTRAIT prefilter and candidate parameters; new scalar names, FLOAT results and ARRAY/ARRAY storage agree across FE/BE. Const/nullable array validation and Hamming range checks precede unsafe access/conversion.
  • Performance and memory: pruned outputs avoid reintroducing embeddings/wide payloads. In-flight and cached row IDs use tracked DorisVector storage, capped at 65,536 survivors; broader predicates discard the entire partial list and keep the complete native filter. Native allowlist identity, copying, empty-set and pre-ranking semantics were verified from the pinned dependency source. Runtime performance was not measured.
  • Other angles: original scan privileges are checked before the late rewrite; output expression identities, qualifiers and vector NOT NULL contracts were traced. Documented native approximate NULL/NaN candidate behavior and the exclusion of inner-NULL vector elements were treated as explicit provider boundaries.

Existing-thread disposition: whitespace preservation, missing-field-ID fallback, native-limit fallback, result nullability, unrelated-schema fallback and row-ID memory tracking are fixed in the inspected head. The existing old-reader compatibility concern remains acknowledged as above. No already-reported P0/P1 finding still applies, so existing_blocking_comment_ids is empty. No duplicate inline comment is added.

User focus: no additional focus was supplied. Remaining validation limits are static-only: no builds, tests, cluster regressions or remote-I/O performance/cancellation measurements were run. No source changes or submodule initialization were performed.

Review converged after 1 round(s).

@hello-stephen

Copy link
Copy Markdown
Contributor

FE Regression Coverage Report

Increment line coverage 74.67% (224/300) 🎉
Increment coverage report
Complete coverage report

@hello-stephen

Copy link
Copy Markdown
Contributor

BE Regression && UT Coverage Report

Increment line coverage 87.36% (152/174) 🎉

Increment coverage report
Complete coverage report

Category Coverage
Function Coverage 76.38% (34510/45183)
Line Coverage 61.21% (389801/636866)
Region Coverage 57.99% (328263/566042)
Branch Coverage 59.01% (151682/257060)

morningman pushed a commit to apache/doris-website that referenced this pull request Oct 23, 2026
Ordinary SQL search over Lance is not described in the current 4.x
reference. This documents the branch-4.2 behavior introduced by
apache/doris#69346 (implementation PR is still
open; this documentation depends on it).

For example, an eligible `ORDER BY
cosine_similarity_approximate(embedding, [0.1,0.2,0.3,0.4]) DESC LIMIT
10` becomes native Lance cosine retrieval, with `1 - _distance`
projected as similarity. Exact functions and ACL/EXISTS queries retain
scalar distance evaluation and global Top-N.

- Add matching English/Chinese guidance for five approximate vector
functions, MATCH/BM25, ordering and result conversion, native versus
scalar NULL/NaN/zero-vector semantics, UInt8 Hamming validation, bounded
row-ID prefilters, and JOIN/EXISTS boundaries.
- Add English/Chinese SQL references for `cosine_distance_approximate`,
`cosine_similarity_approximate`, `hamming_distance`, and
`hamming_distance_approximate`, including normal, zero/empty, NULL, and
out-of-range examples.
- Add navigation entries, link from Lance Catalog, and clarify that
ordinary SQL rewritten to search TVFs can qualify for two-phase output
reads.

Version scope: this change intentionally updates **4.x English and
Chinese only**. The implementation targets `branch-4.2`, not Doris
master; current/development and older-version pages are not being
advertised as supporting it. This explains the governance
version-counterpart warnings. Availability wording requires a Doris 4.2
build containing this capability, rather than implying every build with
the explicit TVFs already supports rewriting.

Validation: all 12 changed documentation pages compiled with the
Docusaurus MDX processor; changed-page link checks found no broken
links; SQL-function structure checks passed; paired English/Chinese SQL
blocks match; all six added sidebar references resolve; whitespace and
added-content disclosure checks passed. SEO descriptions satisfy the
configured length range. Remaining SEO duplicate-title warnings reflect
shared SQL identifiers across locales and existing versioned titles.
Full-repository sidebar lint reports unrelated existing findings. Full
site build and live SQL execution were not run.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants