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 complete for PR #68690 at head 5b793d8. Two full review rounds covered all 13 changed paths; the second normal BE, normal FE, and risk-focused scans each returned NO_NEW_VALUABLE_FINDINGS. The final sweep found one [P3] documentation issue, raised inline. There are no existing inline threads or still-applicable P0/P1 comments. No additional user review focus was supplied.
Critical checkpoint conclusions:
- Goal and proof: The FE accepts query_parallelism in the signed range -1 through INT_MAX, carries it in optional Thrift field 5, and displays explicit values in EXPLAIN. BE validates and applies it to the common Lance vector scanner before execution. Second-phase fetch profiles now carry calls, fetched rows, and cache bytes. Added FE/BE/regression tests cover option values, result equivalence, and profile plumbing; this review inspected them but did not execute them.
- Scope: Changes are localized to the vector option, row-fetch profile path, documentation, and focused tests. No unrelated behavior change was substantiated.
- Concurrency and locks: The option controls concurrency inside each Lance scanner. Parallel row-fetch tasks write distinct blocks/statistics entries and the RPC waits before aggregation. No new shared mutable state, lock order, heavy locked operation, or deadlock was found.
- Lifecycle and static initialization: Reader close samples cache statistics before releasing its dataset handle; counters are copied afterward and merged once per successful RPC. The new fixed-name map has no cross-translation-unit initializer dependency. The framework times production materialization push, including the synchronous RPC wait, once.
- Configuration and compatibility: This is a per-query option, not a process configuration item. Optional Thrift field 5 is additive for rolling upgrades; older BEs may ignore the tuning hint while preserving query semantics. No stored format changed.
- Parallel paths and conditions: The shared VECTOR setup covers indexed, uncovered-fragment/flat, single-vector, and multi-vector scans. FE and BE reject values below -1; empty row-ID takes skip the new counters. No missed functional branch or unsafe conditional was substantiated.
- Tests and expected results: FE tests inspect unset/set state, invalid bounds, Thrift round trip, and EXPLAIN. BE tests inspect invalid setup, result equality, repeated/empty takes, and response profile strings. The regression uses ordered comparisons and expected-error blocks; no .out result changed. These tests do not prove actual parallel speedup or an end-to-end duplicate-ID profile, so runtime validation remains a coverage limit.
- Observability: The option is visible in EXPLAIN and the fetch counters use count/byte units across RPC serialization. The inline finding corrects the documented meaning of LanceRowIdFetchRows when requested IDs repeat. Existing warnings cover failed cache-stat sampling; no additional metric defect was proven.
- Transactions, persistence, and writes: The changed path only reads a fixed Lance snapshot and profiles fetches; it adds no transaction, EditLog, write, or crash-recovery operation.
- FE/BE transport: The new variable is carried in the external search request and copied into each planned split. The pinned Lance C API accepts the same -1/0/positive semantics.
- Performance and other risks: The setter runs per scanner and the four profile lookups run per fetch, outside row loops. No concrete hot-path regression or other unresolved suspicious point remained after convergence.
Review method: static inspection only, as required; no builds, tests, submodule initialization, or product-source edits were performed.
| | Counter | Scope | | ||
| | --- | --- | | ||
| | `LanceRowIdFetchCalls` | Non-empty dataset `take_rows` calls. | | ||
| | `LanceRowIdFetchRows` | Rows converted from successful returned batches, including duplicate requested row IDs. | |
There was a problem hiding this comment.
[P3] Correct the row-fetch count semantics for duplicate IDs
read_external_row_from_file_mapping deduplicates requested IDs before take_rows, then maps duplicate result positions back to the one fetched row. For a request containing the same ID twice, LanceRowIdFetchRows reports 1 while the result has 2 rows, contrary to this description. Describe the metric as distinct rows fetched per dataset/RPC, or count output copies separately.
FE Regression Coverage ReportIncrement line coverage |
|
run buildall |
|
/review |
|
Fixed the BE UT crash in build 1061662 in bfbbbaa. The fixture now registers a real tuple descriptor and supplies matching tuple/nullability lists. The original assertion that direct Validation: the updated test translation unit passes C++ syntax checking, and all C/C++ files changed by this PR pass clang-format 16. Full BE UT execution is pending the newly requested CI run; the locally available BE UT binary is from a different revision. |
There was a problem hiding this comment.
Review complete at head bfbbbaa. Static review only: no builds or tests were run. No new inline findings. The existing P3 comment 4152019381 still applies to the duplicate-ID description of LanceRowIdFetchRows in docs/lance-ann-profile.md:91; it was not reposted. There are no existing P0/P1 inline blockers.
Critical checkpoints:
- Goal and tests: query_parallelism is parsed in FE, carried by optional Thrift through split planning, validated in BE, and applied to each Lance vector scanner before execution. FE, BE, and regression tests cover accepted/rejected values, serialization, EXPLAIN, result preservation, fetch-counter accumulation, and timing behavior. These tests were inspected, not run; they do not measure actual Lance worker concurrency.
- Scope and lifecycle: edits are localized to the vector request, phase-two fetch profiles, operator timing, tests, and documentation. The phase-two reader is closed after its Arrow stream is consumed; dataset-handle cache statistics are captured before the handle closes and aggregated after parallel fetch tasks complete. Every production materialization push is timed once by StatefulOperatorX, including its synchronous RPC wait.
- Concurrency and memory: Lance owns the new partition-search concurrency; no new Doris-owned thread, shared mutable field, lock, or lock order was introduced. Per-task fetch profiles and statistics occupy distinct slots before aggregation. The requested concurrency is capped by Lance's execution resources; no independent memory reservation change is introduced here.
- Configuration and compatibility: query_parallelism is a per-query option, not a dynamic server configuration. The pinned Lance C API supports -1, 0, and positive values; invalid values and setter failures return errors. An omitted optional Thrift field preserves existing behavior, and older BE readers can ignore this performance setting during a rolling upgrade without changing result semantics.
- Parallel paths and conditions: indexed and flat vector splits use the same scanner configuration path; explicit -1/0/positive values and the empty-ID fetch path were checked. FE/BE request propagation is complete. The duplicate-ID counter wording is the already-reported P3 issue.
- Tests, observability, and performance: new profile counters preserve count versus byte units through the RPC and backend profile; the nested ExecTime sample is removed. Test expectations and regression ordering were inspected. Live Foyer byte attribution and actual worker utilization are not independently demonstrated by those tests. No additional actionable performance or observability defect was substantiated.
- Transactions, persistence, data writes, storage formats, and failover: none are modified by this PR. No additional correctness, error-handling, or lifecycle issue was substantiated in the reviewed paths.
All 13 changed files were swept. All three round-1 reviews returned NO_NEW_VALUABLE_FINDINGS, every initial risk has a conclusion, and no candidate remains unresolved. There was no additional user-provided review focus.
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
FE UT Coverage ReportIncrement line coverage |
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
FE Regression Coverage ReportIncrement line coverage |
What problem does this PR solve?
Lance vector searches cannot configure the SDK's query parallelism from Doris, and second-phase row-ID fetch profiles omit available read counters. Materialization also nests two timers for the same
ExecTimecounter, counting the synchronous fetch wait twice.This PR targets
branch-4.1and:vector_search("query_parallelism" = "4", ...)support through FE validation, Thrift, and the existing Lance-C API. Accepts-1,0, and positive integers; omitting it preserves the SDK default. EXPLAIN shows an explicit setting.RowIDFetcherprofile, preserving their units. Statistics are captured after the reader closes.The read-byte counters exclude metadata/index IO and block-alignment amplification. The current take API does not expose physical request counts or a separate decode timer; zero Foyer counters on bypassed paths do not mean zero physical IO. These boundaries are documented in
docs/lance-ann-profile.md.No third-party dependency or patch changes are required.
Validation
Release note
Add configurable Lance vector query parallelism and row-ID fetch read counters, and correct materialization execution-time accounting.
Check List (For Author)
docs/lance-ann-profile.mdin this PR.Check List (For Reviewer who merge this PR)