Skip to content

[improvement](lance) Expose query parallelism and complete row fetch profiles (branch-4.1) - #68690

Open
Gabriel39 wants to merge 2 commits into
apache:branch-4.1from
Gabriel39:dev/lance-query-parallelism-profile-4.1
Open

Gabriel39 wants to merge 2 commits into
apache:branch-4.1from
Gabriel39:dev/lance-query-parallelism-profile-4.1

Conversation

@Gabriel39

Copy link
Copy Markdown
Contributor

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 ExecTime counter, counting the synchronous fetch wait twice.

This PR targets branch-4.1 and:

  • Adds optional 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.
  • Carries row-ID fetch call counts, returned row counts, and per-dataset Foyer cache/origin logical read bytes through the fetch RPC into each RowIDFetcher profile, preserving their units. Statistics are captured after the reader closes.
  • Removes the nested materialization execution timer; the operator framework continues to count the fetch wait once.

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

  • 43 focused FE tests passed, including option bounds, omitted defaults, Thrift round trips, and EXPLAIN propagation.
  • FE Checkstyle passed with zero violations.
  • Clang-format 16 checks passed for every changed C++ source/header.
  • C++ syntax checks passed for all changed translation units and both affected BE test files using regenerated Thrift definitions.
  • Groovy compilation passed for the changed regression suite.
  • Added BE tests for parameter validation, result preservation, row-fetch counters, RPC profile propagation, and avoiding duplicate execution timing.
  • Added external regression coverage comparing indexed results across parallelism settings and rejecting invalid values. Full BE UT and external regression execution remain for CI; local C++ checks were syntax-only.

Release note

Add configurable Lance vector query parallelism and row-ID fetch read counters, and correct materialization execution-time accounting.

Check List (For Author)

  • Test
    • Regression test
    • Unit Test
  • Behavior changed:
    • Yes. Explicit query parallelism is now accepted; existing defaults are preserved. Materialization profiles no longer double-count fetch execution time.
  • Does this need documentation?
    • Yes. Updated docs/lance-ann-profile.md in this PR.

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 1, 2026 04:41
@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 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.

Comment thread docs/lance-ann-profile.md
| Counter | Scope |
| --- | --- |
| `LanceRowIdFetchCalls` | Non-empty dataset `take_rows` calls. |
| `LanceRowIdFetchRows` | Rows converted from successful returned batches, including duplicate requested 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.

[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.

@hello-stephen

Copy link
Copy Markdown
Contributor

FE Regression Coverage Report

Increment line coverage 87.50% (7/8) 🎉
Increment coverage report
Complete coverage report

@Gabriel39

Copy link
Copy Markdown
Contributor Author

run buildall

@Gabriel39

Copy link
Copy Markdown
Contributor Author

/review

@Gabriel39

Copy link
Copy Markdown
Contributor Author

Fixed the BE UT crash in build 1061662 in bfbbbaa. MaterializationOperatorTimingTest.PushDoesNotDuplicateFrameworkExecTimer constructed an operator with an empty row_tuples list, triggering RowDescriptor's DCHECK_GT(row_tuples.size(), 0) before reaching the timing assertion.

The fixture now registers a real tuple descriptor and supplies matching tuple/nullability lists. The original assertion that direct push() calls must not increment the framework-owned timer is preserved; no production behavior changes.

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.

@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.

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.

@hello-stephen

Copy link
Copy Markdown
Contributor

BE UT Coverage Report

Increment line coverage 48.28% (14/29) 🎉

Increment coverage report
Complete coverage report

Category Coverage
Function Coverage 61.18% (26787/43781)
Line Coverage 45.81% (278299/607460)
Region Coverage 41.50% (220432/531205)
Branch Coverage 42.98% (102196/237752)

@hello-stephen

Copy link
Copy Markdown
Contributor

FE UT Coverage Report

Increment line coverage 100.00% (8/8) 🎉
Increment coverage report
Complete coverage report

@hello-stephen

Copy link
Copy Markdown
Contributor

BE Regression && UT Coverage Report

Increment line coverage 58.62% (17/29) 🎉

Increment coverage report
Complete coverage report

Category Coverage
Function Coverage 74.91% (31892/42576)
Line Coverage 59.24% (356934/602564)
Region Coverage 55.95% (297869/532339)
Branch Coverage 56.76% (134785/237469)

@hello-stephen

Copy link
Copy Markdown
Contributor

FE Regression Coverage Report

Increment line coverage 87.50% (7/8) 🎉
Increment coverage report
Complete coverage report

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.

2 participants