Repository navigation
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
|
/review |
There was a problem hiding this comment.
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).
FE UT Coverage ReportIncrement line coverage |
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
FE Regression Coverage ReportIncrement line coverage |
69268e3 to
16f8c09
Compare
|
run buildall |
|
/review |
There was a problem hiding this comment.
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:
- 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.
- Scope: all 12 changed paths were reviewed. The change is focused on the rewrite, bound TVF factories, bounded row-ID prefilter, registration, and tests.
- 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.
- 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.
- Configuration: no configuration item was added. The existing fe_debug/fuzzy-mode interaction is the inline issue.
- 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.
- Thrift/protobuf: no definitions changed; no new field/enum IDs require cross-branch comparison.
- 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.
- 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.
- 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.
- Test results: no golden result files changed; expected values and comparison/tolerance assertions in the changed tests were inspected.
- Observability: new collection time, candidate count, cache-hit, and broad-fallback counters supplement existing native scan statistics. No additional correctness issue was confirmed.
- Persistence/failover: no transaction or EditLog mutation changed; scans use a pinned immutable dataset snapshot.
- Data writes/atomicity: no write path changed. Cancellation, errors, EOF, and reader cleanup were inspected.
- 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.
- 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.
- 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).
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
FE Regression Coverage ReportIncrement line coverage |
|
run buildall |
|
/review |
There was a problem hiding this comment.
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).
|
run buildall |
|
/review |
There was a problem hiding this comment.
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:
- 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.
- Scope: the late rewrite, bound-scan factories, BE prefilter and tests remain focused and reuse existing search planning/execution.
- 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.
- 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.
- Configuration: no new configuration was added; the extra scanner uses existing scan options.
- 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.
- Definitions: no Thrift/protobuf definitions, field IDs, enum values or storage formats change.
- 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.
- 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.
- 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.
- Results: no expected-output files change; new assertions are consistent with the inspected fixture and planning contracts. No passing test result is claimed.
- 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.
- Persistence: no EditLog, commit, replay, master-failover or partition-visibility path changes.
- Writes: the change is read-only; no transaction/data-write atomicity changes. Partial prefilter lists are discarded rather than published as complete results.
- 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.
- 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.
- 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).
|
run buildall |
|
/review |
Local pipeline review — ✅ PASSschema: 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: trueNotes for maintainers
Reviewed locally with the |
There was a problem hiding this comment.
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:
- 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.
- Scope and reuse: the changes are focused and reuse existing search TVFs, native scanner APIs, and scan-local caching.
- 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.
- 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.
- Configuration: no configuration item is added; existing read-ahead and session controls are reused.
- 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.
- Shared definitions: no gensrc/thrift or gensrc/proto file changes, so cross-release field-ID/enum comparison is inapplicable.
- 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.
- 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.
- 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.
- Expected results: no golden output file changes. Inspected row, score, metric, nullability, and ExprId assertions agree with the intended contracts; execution is unverified.
- Observability: EXPLAIN retains kind/metric/version/branch; new prefilter timers, survivor counts, hits, and fallbacks complement existing native IO/index counters.
- Persistence and failover: no transaction, EditLog, replay, or persisted metadata mutation is added.
- Writes and atomicity: the feature is read-only; write/crash-atomicity behavior is unchanged.
- 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.
- 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.
- 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).
| 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; |
There was a problem hiding this comment.
[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.
There was a problem hiding this comment.
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.
|
run buildall |
|
/review |
There was a problem hiding this comment.
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:
- 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.
- Scope and reuse: the rewrite, bound factories, reader prefilter and scalar additions are focused and reuse existing planning/execution and distance templates.
- 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.
- 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.
- Configuration: no new item or dynamic-propagation requirement; existing scan options are reused.
- Compatibility: no persisted format change; matching FE/BE function registrations are present. Old-reader compatibility remains the existing limitation described above.
- Thrift/proto: no definitions changed, so release-branch field/enum allocation checks are inapplicable.
- 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.
- 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.
- 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.
- Expected results: no output golden files changed; new assertions align with the declared scalar/native metric and candidate contracts on inspection.
- Observability: prefilter time, rows, cache hits and fallback counters supplement existing scan statistics and explain information; no new per-row logging.
- Persistence and failover: read-only change, no EditLog or transaction mutation; bound version/URI/schema checks preserve snapshot identity.
- Writes and crash cleanup: no data-write atomicity path changed; temporary resources and scan-local cache ownership have explicit cleanup.
- FE-BE passing: existing search requests carry metrics, query values, candidate bounds, prefilter and SUBSTRAIT payload through split serialization and reader validation/application.
- 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.
- 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).
|
run buildall |
|
/review |
Local pipeline review — ✅ PASSschema: 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: trueNotes for maintainers
Reviewed locally with the |
There was a problem hiding this comment.
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:
- 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.
- 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.
- 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.
- 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.
- Configuration: no new production configuration is introduced; existing native scan options apply to the collector.
- 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.
- Shared definitions: no Thrift/protobuf definitions changed, so cross-release ID comparison is inapplicable.
- 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.
- 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.
- 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.
- 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.
- Observability: existing search Explain/profile information plus prefilter time, survivor, hit and fallback counters cover the new path without per-row INFO logging.
- 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.
- Writes and atomicity: the change is read-only planning/scanning and stateless scalar evaluation; no data-write atomicity protocol changes.
- 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.
- 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.
- 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).
FE UT Coverage ReportIncrement line coverage |
FE Regression Coverage ReportIncrement line coverage |
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
|
run buildall |
|
/review |
There was a problem hiding this comment.
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).
FE Regression Coverage ReportIncrement line coverage |
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
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.
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 explicitvector_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 revision4fafcd64525b274b113208390bbcca86bbbc093bfrom #69332 is already included in the branch-4.2 base; this PR no longer duplicates the dependency upgrade.Function and metric coverage
l2_distance_approximate(v, q)l2sqrt(native squared L2), smaller is betterinner_product_approximate(v, q)dot(dot_productalias in the TVF)1 - native dot distance, larger is betterl2_distance(v, q)/inner_product(v, q)l2/dotcosine_distance_approximate(v, q)(new)cosinecosine_similarity_approximate(v, q)(new)cosine1 - native cosine distance, larger is betterhamming_distance_approximate(v, q)(new)hammingcosine_distance(v, q)/cosine_similarity(v, q)/hamming_distance(v, q)(new exact Hamming function)cosine/hammingl1_distance(v, q)score()withMATCH_ANY/MATCH_ALL/MATCH_PHRASEThis PR covers five vector SQL functions with the
_approximatesuffix: L2, inner product, cosine distance, cosine similarity, and Hamming distance. The three cosine/Hamming approximate functions and exacthamming_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 asnprobes,refine_factor, andefremain 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 forl2,1 - dotfordot, cosine distance forcosine, and Hamming distance forhamming. 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
_approximatefunction 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.assert_true()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
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:
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:
Hamming queries target binary UInt8 vectors. For example, on a three-byte embedding:
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
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_ANYmaps to a match query with OR.MATCH_ALLmaps to a match query with AND.MATCH_PHRASEmaps to a phrase query using the stored analyzer and position information.AND score() >= 0.5is 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:
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
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_idvalues into Lance_rowid, and it does not consume dynamically produced semi-join IDs. Supporting that scenario requires a separatedoc_id -> native _rowidlookup 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
SEARCH()DSL.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.limit + offsetexceeds 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
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 withfe_debugenabled and disabled, including ancestor projections; the external regression suite also executes vector queries withfe_debug=true.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 usearray(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)
Check List (For Reviewer who merge this PR)