Repository navigation
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Two P2 issues need attention: cold vector-index prewarm can confuse an index name with another column, and qualified projections are rejected when FROM omits the matching default catalog/database. Both are detailed inline. No P0/P1 finding or existing blocking inline comment was found.
Reviewed all 23 changed paths and traced SQL parsing/execution, authorization, compute-group/backend selection, snapshot/credential resolution, bounded metadata work, FE RPC fanout, BE admission/lifetime, and the exact pinned Lance-C/Lance prewarm implementation through the query session cache.
| Critical checkpoint | Conclusion |
|---|---|
| Goal and proving tests | Implements synchronous, snapshot-pinned index warming through WARM UP SELECT; the two inline cases remain incorrect and are absent from the new tests. |
| Change scope and clarity | Additive index mode and RPC preserve the existing data-warmup branch. Ancillary regex-timing and Flight timeout changes are test-only. |
| Concurrency and locks | FE metadata work is bounded to 4 active/16 queued; fanout is at most 8 per wave. BE uses one active/one queued worker and a nonblocking admission mutex. No new lock-order cycle found; long SDK IO is isolated from shared RPC workers. |
| Lifecycle and static initialization | Catalog leases and worker-owned Dataset/allocator lifetimes survive abandoned waits; BE RAII and the RPC completion closure retain resources through completion. Shared session cache ownership is preserved. No new cross-TU initialization dependency. |
| New configuration | None. Existing cache and backend-selection settings are reused. |
| Interface/storage compatibility | No persisted-format change. New optional RPC fields and strict status/version acknowledgement make unsupported old BEs fail explicitly. |
| gensrc release comparison | Read touched definitions on HEAD, master and branches 3.1/4.0/4.1/4.2; new messages/method are absent elsewhere, with no conflicting IDs or required fields. |
| Parallel paths | Cloud/non-cloud group selection, ordinary data warming, scalar/vector/FTS SDK paths, and text/binary prepared execution reviewed. The two inline findings identify the affected exceptional paths. |
| Conditional checks | Column/star restrictions, aliases, WHERE/EXPLAIN rejection, positive fixed versions, C-string NUL checks, deadline admission and success acknowledgement reviewed. Qualifier validation needs the inline fix. |
| Test coverage | FE tests cover auth, grouping, RPC fanout/errors, deadlines/cancellation and prepared metadata. BE tests cover shared cache/version, redaction and queue bounds. Regression covers query equivalence, retries, all indexes, data mode and per-BE failure. Add the two inline edge cases, especially a cold vector cache before any query. |
| Changed expected results | No generated golden output changed. Test assertions and ancillary test changes were checked statically. |
| Observability | Index/version/backend count/elapsed result fields, contextual FE failures and existing session-cache metrics remain available; no per-row logging added. |
| Transactions and persistence | No EditLog, rowset, delete-bitmap, commit/visibility or failover state is changed. The pinned external snapshot is read-only. |
| Writes and crash behavior | No table-data mutations; cache fills are expendable. Native work may finish after caller cancellation, bounded and owned until return. |
| FE-BE field propagation | URI, version, logical index, storage options and remaining timeout reach the new RPC; every successful response must acknowledge the same version. |
| Performance | Setup and field selection occur once per metadata snapshot; fanout/native work are bounded and use the existing shared cache. No new per-row scan work. |
| Other concerns | Quoted aliases, prepared administrative target identity, cache reuse, RPC ownership and old-BE acknowledgements were investigated and dismissed with code evidence; no unresolved static candidate remains. |
No additional user focus was supplied. Validation is static only: no builds, tests, source edits or submodule initialization were performed. Distributed cloud/REST, mixed-version execution, real RPC cancellation during native IO and cache residency were not exercised.
Convergence: converged in 2 review rounds. All three reviewers returned NO_NEW_VALUABLE_FINDINGS on the same frozen two-comment set; full changed-path coverage and every candidate/risk disposition were verified. Delivery type: COMMENT with two P2 findings.
Review converged after 2 round(s).
|
run buildall |
|
/review |
There was a problem hiding this comment.
Static review of head 4339941ee71713ae7a171f0652aa5adf8869357d: two new P2 findings affect which Lance table is warmed. All 25 changed paths were reviewed, with FE, BE and separate SDK risk reviews. No existing P0/P1 finding applies. The reported triggers in existing threads 4312204893 (direct default qualification) and 4312204876 (SDK index/column name collision) are fixed; neither is reposted. F1 concerns a later session-context change, and F2 concerns a catalog case-policy override.
Critical checkpoints:
- Goal and evidence: index-only warmup pins metadata/version/access, warms the shared query-session cache and waits for all selected backends. The two inline findings expose target-identity gaps. Tests were inspected statically.
- Scope: parser/command, metadata/RPC orchestration, BE session/service, SDK patch and downloader are covered. Timing-assert removals retain correctness assertions; the Iceberg test restores its prior Flight timeout.
- Concurrency: metadata pool 4 workers/16 queued, FE fanout at most 8, BE dedicated pool 1 running/1 queued; manager try-lock bounds prewarm without serializing query opens. Deadline/cancellation and lock order were traced; no additional defect found.
- Lifecycle/static initialization: worker-owned Dataset/allocator/client leases, immutable snapshot/access, C++ RAII and SDK session/snapshot Arcs retain native owners through completion. Request lifetime and server/service shutdown ordering were checked; no new cross-TU initialization dependency.
- Configuration: no new setting; existing session-cache configuration remains restart-scoped, while backend selection reads current configuration.
- Compatibility: no persisted-format change. Unsupported old-BE RPCs, missing status and missing/wrong version acknowledgements fail explicitly during rolling upgrades.
- Shared definitions: complete touched protobuf definitions compared with master and branches 4.2/4.1/4.0/3.1; new messages/method are additive, fields optional/map, no conflicting IDs or required fields found.
- Parallel paths: index/data modes, cloud/shared-nothing compute-group selection, prepared/binary results and normal-query shared-cache reuse checked. Modern/legacy vector and scalar/FTS dispatch retain their underlying implementations.
- Conditions/errors: unsupported projections/properties, missing indexes/field IDs, expired queues and embedded NULs fail explicitly. The two findings concern identity resolution before these checks.
- Test coverage: FE authorization/selection/fanout/deadline/cancellation/prepared tests, BE session/admission tests, SDK fresh-session multi-segment/collision tests and both regression suites inspected. Neither new target-identity case is covered. Native post-timeout completion, NUL/provider exception branches and broader legacy/family interoperability lack direct new runtime coverage.
- Expected results: no golden output changes; deterministic query ordering, explicit version/status/cache assertions, test registration and fixture visibility checked.
- Observability: shared cache statistics and per-index elapsed/backend-count results remain available; Doris boundary errors preserve operation context and suppress provider causes. No new actionable observability defect found.
- Transactions/persistence: no EditLog, transaction publication, visible-version or delete-bitmap changes; not applicable to these read-only cache fills.
- Writes/crashes: no new external-data write or atomicity path. Already-started native IO can continue after caller cancellation/timeout, with bounded admission and retained ownership.
- FE-BE propagation: URI, storage options, logical index, fixed version and remaining timeout reach the handler intact; success requires the explicit matching version from every target.
- Performance: no per-row scan changes; bounded metadata/RPC/native admission, shared cache reuse, existing vector windows/scalar chunking and strict FTS residency retained. Patch-set fingerprinting invalidates stale/partially patched sources.
- Further recheck: all initial credential/snapshot, SDK family/cache and noninterruptible-IO risks have evidence-backed dispositions. Both new findings are separate and actionable; no other concrete candidate remains unresolved.
No additional user focus was supplied. Review validation is static only: no builds, tests, downloader/patch execution or submodule initialization were performed. Runtime race, cache-capacity and interoperability behavior remains outside the executed evidence.
Review converged after 3 rounds: every assigned reviewer returned NO_NEW_VALUABLE_FINDINGS on the complete unchanged two-comment set; all changed paths, candidates and initial risks have explicit dispositions. No unresolved concrete review question remains.
Review converged after 3 round(s).
FE UT Coverage ReportIncrement line coverage |
FE Regression Coverage ReportIncrement line coverage |
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
4339941 to
7a861e1
Compare
|
run buildall |
|
/review |
There was a problem hiding this comment.
Static review of PR #69277 at 7a861e1245306e7a81342eb5a2eb851a8c68a7b1 found three new P2 issues: retained table defaults across prepared executions, omission of usable legacy scalar indexes, and the outdated third-party patch harness. All are inline. No new P0/P1 finding was verified.
All 25 changed paths and the complete authoritative diff were reviewed, with independent FE, BE/dependency and risk-focused reviewers. No extra user focus was supplied. This was static analysis only: no builds, tests, source modifications or submodule initialization were performed. Checked-in tests and pinned upstream implementations were inspected; author-reported test runs were not reproduced.
Existing review context was revalidated without duplicate comments. P2 threads 4312669339 (parse-time projection qualification under a context change) and 4312669354 (global normalization before catalog case policy) still apply. The direct omitted-default qualification case in 4312204893 and vector-index/column-name collision in 4312204876 are addressed by the current parser/dependency patch. There are no existing P0/P1 IDs to carry forward. The new prepared-state finding concerns the retained command after an earlier execution, uses an unqualified star and requires a separate fix from projection qualification.
| Critical checkpoint | Conclusion |
|---|---|
| Goal and proof | Main synchronous fixed-snapshot path is present; prepared-default and legacy-discovery findings prevent complete intended behavior. Modern-path unit, integration and dependency tests are present and were inspected. |
| Scope and clarity | Production changes follow grammar, command, Lance coordinator, RPC and shared-session prewarm. The two unrelated test stabilizations preserve exact correctness assertions. The downloader's downstream harness migration is missing. |
| Concurrency and locks | FE metadata work has four workers and sixteen queue slots; RPC fanout is at most eight. BE uses one dedicated worker and one queued request, with a nonblocking manager admission lock. No new query-lock or lock-order defect was substantiated. |
| Lifecycle and initialization | Lazy singleton/call_once, metadata generation leases, worker-owned allocator/Dataset, RAII and SDK Arc snapshots preserve ownership through caller cancellation. The retained mutable prepared table name is the reported lifecycle defect. No new static-initialization cycle was found. |
| Configuration | No config is added; existing native cache restart semantics and session/group selection remain. |
| Compatibility | No persisted-format change. New RPC is additive; unsupported older BEs and absent/wrong acknowledgements fail explicitly. |
| Protobuf definitions | New messages/RPC were compared with master and release 3.1/4.0/4.1/4.2 snapshots: absent there, fresh optional/map tags, no reused IDs or new required fields. |
| Parallel paths | Ordinary data warmup/file-cache admission remains. Five-column index-mode PREPARE metadata, scalar/FTS/vector paths, collision handling and query-session reuse were traced. |
| Guards and errors | Property/projection checks, positive pinned version, exact acknowledgement, NUL rejection, deadlines and capacity errors fail explicitly. Provider errors are sanitized. |
| Test breadth | Modern selection, multisegment cold-session collision, snapshot/session reuse, auth/group, cancellation/expiry/admission and data-mode coverage exist. Legacy discovery and repeated prepared execution across USE lack coverage; the existing patch harness is broken as reported. |
| Expected results | No .out edits. Ordered vector comparisons and dynamic version/backend assertions are coherent; timing-assertion removals preserve exact match counts, and the Flight probe restores its prior query timeout. |
| Observability | Result rows expose table/index/version/backend count/elapsed; failures retain index/version/backend context. Existing session cache and metadata metrics remain. |
| Transactions and persistence | No EditLog, rowset, internal visibility or delete-bitmap path changes. External snapshot metadata is read only. |
| Writes and crash atomicity | Only process-local caches are filled. Partial warming after an explicit command failure does not modify dataset contents. No new persistent atomicity defect was found. |
| FE-to-BE propagation | URI, positive version, logical index, options and remaining timeout reach every target through the sole new administrative RPC; returned version is checked. |
| Performance and memory | Fanout, metadata jobs and retained native operation counts are bounded. Existing configured shared caches are reused. Native I/O remains uninterruptible; no runtime memory, eviction or throughput measurements were made. |
| Further sweep | Every candidate/risk focus has an accepted, duplicate or evidence-backed dismissal. All changed paths are covered; no unresolved code question remains. |
Coverage limits are the static-only validation above; runtime integration, cold-cache behavior, provider hangs and performance were not executed or measured in this invocation.
The review converged in two rounds: all three reviewers returned NO_NEW_VALUABLE_FINDINGS on the unchanged final set, with complete coverage and no unresolved candidate/risk.
Review converged after 2 round(s).
`lance_dataset_prewarm_index()` can fail for a valid vector index when its name matches another schema column. The C++ `Dataset::prewarm_index()` wrapper consequently throws `lance::Error`. Resolve the indexed field from segment metadata for this collision so prewarming succeeds on a fresh session and subsequent queries reuse the warmed partitions. ### Trigger and impact A concrete reproducer is: 1. Create a dataset with `id: Int32` and `embedding: FixedSizeList<Float32, 8>`. 2. Build an `IVF_FLAT` index on **`embedding`**, but name the index **`id`**. The regression uses two fragments and two index segments. 3. Open the dataset with a fresh session, before any vector query has warmed that session. 4. Call `lance_dataset_prewarm_index(dataset, "id")`. Before this fix, the call returns `-1` with an error containing `Data type is not a vector ... but Int32`. Index construction and nearest-neighbor queries on `embedding` succeed for the same dataset. Consequently, callers cannot complete an explicit warmup before serving queries; applications that continue after the error still incur cold index reads. Applications that require warmup to succeed can reject an otherwise usable dataset. The demonstrated failure is in prewarm, without changing persisted data or index contents. The name collision matters: an index name that is not a schema column already resolves through index metadata. The test uses a separate session for the cold control query, so that query cannot populate the session used to reproduce prewarm. ### Cause and fix Reproduced against lance-c `2089f6f` (current `main`), which pins Lance SDK `68c12df`. This is a correctness follow-up to #94. The pinned Lance SDK passes the logical index name to `open_generic_index()`. Its vector field resolver checks for a schema column with that name before consulting the segment's field ID, so `id` incorrectly selects the scalar column instead of `embedding`. For a name that matches a different schema field, the binding loads the named index's segment metadata and opens each affected vector segment using the indexed field path and segment UUID. It prewarms that opened index on the same dataset snapshot and session. The complete open-and-prewarm operation for each affected segment runs concurrently via `try_join_all`, preserving the SDK’s fail-fast error propagation and cancellation of pending segment futures. Other paths continue through the SDK, including missing-index errors and scalar/full-text prewarm with its aggregate residency validation. This keeps the existing C/C++ API and dependency revision. It does not include the Doris-specific Foyer patch. Found while integrating upstream prewarm into [apache/doris#69277](apache/doris#69277 (comment)). ### Validation Linux, Rust 1.97.0, using the unchanged lockfile and default features: - Before the fix: the new cold-session collision test failed with `-1` and the `Int32`/vector type error, after its independent cold-query control succeeded. - `cargo test --locked --test c_api_test test_prewarm_index`: all 6 prewarm tests passed. The vector cases cover non-colliding names, names matching the indexed field, and names matching a different scalar field. Both segments return correct nearest neighbors with **zero partition reloads** after prewarm and a shared-session reopen. - `cargo test --locked --lib index::tests::`: all 3 scheduling tests passed. Deterministic gates verify that all 8 segment opens start before any may finish, and all 8 prewarms start before any may finish. Separate checks verify immediate error propagation/cancellation during open or prewarm, and scalar/unresolved-segment fallback to the SDK. The concurrency tests failed against the serial implementation (`1` segment started instead of `8`); no timing thresholds or sleeps are used. - Coverage also includes B-tree/FTS name collisions, invalid arguments, missing indices (including names matching an existing column), pinned snapshots, repeated calls, and dataset/session handle lifetimes. - `cargo test --locked`: 492 passed, 0 failed; the 3 ignored C/C++ cases were run separately. Doc-tests also completed successfully. - `cargo test --locked --test compile_and_run_test -- --ignored --test-threads=1`: all 3 C/C++ compilation/runtime cases passed. - `cargo clippy --locked --all-targets -- -D warnings`: passed. - `cargo fmt -- --check` and `git diff --check`: passed.
Replace the local prewarm field-resolution patch with the pinned upstream 4fafcd64525b274b113208390bbcca86bbbc093b revision from apache#69332. Restore the existing Foyer-only downloader and refresh its patch context. Validated the downloader patch/cache harness, locked Cargo dependency resolution, patched C/C++ test source syntax, and shell/diff checks.
|
run buildall |
|
/review |
There was a problem hiding this comment.
Reviewed PR #69277 at d594a05 using static analysis of all 25 changed paths and their affected callers/dependencies. One new P3 finding is inline: overqualified FROM names silently lose their leading components and target another table. No new P0/P1 was verified.
Existing review context was independently rechecked and deduplicated. Four P2 threads still apply: execution-time projection qualification (4312669339), catalog-specific table case policy (4312669354), prepared execution retaining table defaults (4320790528), and legacy scalar-index discovery (4320790531). These are already reported and are not reposted. No existing P0/P1 needs carrying forward. Default projection qualifiers, the colliding vector-index name, and the obsolete combined-marker/harness issue are fixed at this head.
Critical checkpoint conclusions:
- Goal and proof: index-only WARM UP SELECT pins one dataset snapshot, discovers selected logical indexes and synchronously warms eligible BEs through the query session cache. Feature tests cover the intended flow, with the known identity/discovery issues above and the new input-validation defect.
- Scope: production changes are focused on parsing, coordination and a dedicated RPC/native entry. CacheHotspot tests retain exact correctness assertions while removing unstable timing thresholds; the Iceberg regression restores the previous query timeout before a fresh scan.
- Concurrency and locks: cancellation is volatile per executor; metadata and RPC fan-out are bounded. BE work uses one dedicated worker plus one queued request and a manager try-lock. Session initialization uses call_once; no new lock-order inversion or shared-worker starvation was verified.
- Lifecycle and static initialization: FE leases and try-with-resources retain JNI Dataset/allocator ownership after caller timeout; BE RAII and ClosureGuard retain dataset/RPC ownership until completion. No new cross-translation-unit initializer dependency was found.
- Configuration: no new configuration item or dynamic-update behavior is introduced.
- Compatibility and definitions: new protobuf fields are optional; touched message/RPC names were compared with master and release branches 4.2/4.1/4.0/3.1 without conflicts. Missing support, missing status and wrong-version acknowledgements fail explicitly. No persisted storage format changes.
- Parallel paths: ordinary data warmup retains its inherited execution and cache requirements; schema-only paths remain separate. Pinned Lance-C keeps vector segment opening/prewarm concurrent and scalar/FTS SDK validation. External scan and prewarm share the session manager and backend eligibility policy.
- Conditions and errors: positive versions, names, embedded NULs, expired queues and projection/property restrictions are checked. The new inline finding identifies the missing relation-name arity check. Provider details are redacted and backend failures remain failures.
- Test coverage: inspected FE parser/auth/selection/deadline/cancellation/prepare tests, BE argument/redaction/shared-session/queue tests, and Lance regression scenarios. Existing prepared-context, case-policy and legacy-index gaps remain; regression queries run before warmup, so they do not independently prove cold vector residency.
- Test results: no golden output changed; added assertions agree with the five-column index-mode schema and existing six-column data-mode schema. No tests or builds were run during this review.
- Observability: result rows identify index/version/backend count/elapsed time; errors include index/version/backend and existing session-cache metrics expose cache effects. No new per-row logging or separately substantiated observability defect.
- Transactions and persistence: no EditLog, commit/replay, master-failover transaction state or durable metadata is changed.
- Writes and crash safety: prewarm changes cache residency only. No data mutation or transaction visibility change; native IO already started may outlive caller timeout while admission remains bounded.
- FE/BE propagation: URI, storage options, fixed version, logical index and remaining timeout use the dedicated request; FE requires the exact returned version and BE verifies the opened version.
- Performance and memory: fan-out is limited to eight; metadata is resolved once; abandoned native work is bounded and isolated. Existing SDK/Rust cache memory-accounting limitations are inherited. Missing worker ThreadContext was investigated without a concrete newly reachable tracked-allocation failure. No benchmark or runtime memory claim is made.
- Further review: complete FE and BE reviews plus an independent cache/lifecycle risk review included missed-problem rechecks, duplicate dispositions and a final changed-path/anchor sweep.
No additional user-provided review focus was supplied. Static dependency validation matched the pinned archive checksum and all 575 Foyer patch contexts; dependency code was not built or executed. Runtime cancellation/stress, cold-cache integration, rolling-upgrade execution and storage-provider behavior remain unexecuted validation limits, not unresolved code candidates. Review convergence is reported separately from the existing P2 issues and does not imply approval.
Review converged after 2 round(s).
Resolve raw table identifiers and projection qualifiers together for every execution. Preserve catalog-specific table spelling, reject overqualified names, and avoid retaining session defaults across prepared executions. Read raw user-index names and field IDs from the same pinned query-access snapshot for prewarm, including legacy scalar indexes without details. Keep planner index discovery unchanged and retain discovery limits. Validation: 113 focused FE unit tests, Checkstyle, the third-party patch harness, and git diff --check passed. The new name-resolution and legacy index regressions failed against the original implementation.
|
run buildall |
|
/review |
There was a problem hiding this comment.
Static review of PR #69277 at 90ed9b4c2a1d329cbecbb9f889430d4934e5b81a found one new P2: a cold BE cannot prewarm a valid legacy scalar index whose logical name differs from its column. The inline comment describes the exact older-writer and pinned-SDK path. All 34 changed paths and their affected callers were covered.
Existing-thread dispositions: execution-time projection/default resolution, repeated prepared execution, catalog case policy, overqualified identifiers, raw legacy-index discovery, modern vector name collisions, and the Foyer marker protocol fixes are present on this head. The new finding concerns native scalar opening after successful discovery, a separate instance from the prior FE omission and modern vector collision. No existing P0/P1 finding remains to carry forward. There was no additional user-provided review focus.
The reviewed execution path is: retained parser identifiers -> execution-local qualification -> ADMIN/SELECT and compute-group authorization -> bounded metadata worker and leased query-access snapshot -> raw index names/field IDs -> bounded per-index backend RPCs -> the shared Lance query session -> status and pinned-version acknowledgement -> synchronous result.
Critical checkpoint conclusions:
- Goal and proof: index-only WARM UP SELECT uses a pinned snapshot and the query cache; the reported legacy scalar case remains defective. Positive and negative FE/BE/regression test sources were inspected; their execution was not verified here.
- Scope and clarity: the dedicated prewarm coordinator and metadata representation keep the new behavior focused. The two unrelated test stabilizations retain correctness assertions and restore the Flight query timeout; no production regression was found there.
- Concurrency and locks: the FE metadata pool is bounded at 4 workers plus 16 queued tasks, RPC fanout at 8, and BE admission at 1 running plus 1 queued request. Caller cancellation is volatile and invocation-local. The manager uses a try-lock; no new lock-order cycle or heavy shared-RPC-worker blocking was found.
- Lifecycle and initialization: metadata workers retain catalog leases and sole ownership of Dataset/allocator cleanup; BE datasets use RAII and share the lazy process Session. Prepared executions use fresh executors. No new ownership cycle, premature close, or cross-translation-unit initialization dependency was found.
- Configuration: no new configuration item is introduced. Existing session file-cache gates continue to apply to ordinary data warmup; index warmup uses the SDK cache settings.
- Compatibility: the command and RPC are additive and introduce no persisted-format change. Unsupported old BEs fail explicitly. Older scalar index compatibility has the inline gap above.
- Shared definitions: touched protobuf definitions and service names were compared with master and branches 4.2, 4.1, 4.0 and 3.1. New fields are optional/map fields; no reused name/tag or changed existing meaning was found.
- Parallel paths: ordinary/default-false warmup, cloud/non-cloud compute groups, text/binary prepared execution, scalar/FTS/vector, and repeated/multisegment prewarm were traced. Queries and prewarm reach the same LanceSessionManager.
- Conditional/error paths: unsupported WHERE/EXPLAIN/projections/properties, invalid qualifiers/fields, absent indexes/backends, queue expiry, NUL-containing C strings, failed SDK calls, and missing/wrong acknowledgements fail explicitly. Provider errors are sanitized while retaining operation identifiers.
- Test coverage: source tests cover authorization, session resolution, field selection, pinning, fanout, cancellation, queue bounds, shared-session reuse and result schema. Missing coverage includes the reported cold legacy default-name scalar case and real REST-vended/managed-namespace end-to-end prewarm.
- Test results: no golden output file changed. New vector comparisons use explicit ordering; index comparisons use deterministic sets. No runtime test result is claimed by this review.
- Observability: per-index results expose version/backend count/elapsed time; existing SDK cache metrics remain. Worker-local metadata profile attribution is absent without caller TLS, but no additional actionable observability defect was established.
- Transactions and persistence: prewarm reads existing metadata/index files and creates no transaction, EditLog entry, or persisted table state; master-failover replay is unaffected.
- Writes and crash safety: only ephemeral cache state changes. Cancellation leaves native resources with their worker until completion; stalled SDK work remains isolated and bounded. No dataset-write atomicity change was found.
- FE-BE propagation: URI, fixed version, logical index name, resolved storage options and remaining timeout cross the sole new RPC path. BE validates the opened version, and FE requires a matching success acknowledgement from every target.
- Performance: prewarm metadata avoids data/fragment scans and planner index descriptions; physical/logical discovery and RPC fanout are bounded. The pinned upstream collision fix retains parallel segment opening/prewarm. No further verified performance defect was found.
- Additional recheck: identifiers/aliases, native scalar/vector fallbacks, FTS validation, cancellation/shutdown, cache identity, dependency defaults and release refs were rechecked. The dependency checksum matches and the retained Foyer patch applies in a zero-fuzz dry run. No additional verified finding or unresolved candidate remains.
Validation limits: this was static analysis only, including inspected test sources and pinned upstream code. No builds, tests, submodule initialization, or repository source modifications were performed. Runtime/native fault-injection, live REST credential vending, and end-to-end performance remain unexecuted. These limits do not leave any assigned changed path uncovered.
Converged in 2 review rounds: all three assigned reviewers returned NO_NEW_VALUABLE_FINDINGS on the unchanged final inline set; every candidate and risk focus has an explicit disposition, and no assigned scope remains uncovered.
Review converged after 2 round(s).
| if (lance_dataset_version(dataset.get()) != version) { | ||
| return Status::InternalError("Lance index prewarm opened an unexpected snapshot"); | ||
| } | ||
| if (lance_dataset_prewarm_index(dataset.get(), index_name) != 0) { |
There was a problem hiding this comment.
[P2] Resolve legacy scalar fields before SDK prewarm
On a cold BE, a Lance v0.18.0 BTREE on id normally has logical name id_idx and no index_details. FE now correctly discovers it, but this call still fails: pinned Lance-C delegates to Dataset::prewarm_index("id_idx") when no schema field is named id_idx; the SDK passes that name through open_generic_index/open_scalar_index to infer_scalar_index_details, which requires schema.field("id_idx"). Thus both WARM UP SELECT id ... and SELECT * fail for a valid index that ordinary scans open using id. This is a BE field-resolution gap after successful discovery, distinct from the previous FE omission. Resolve scalar fields from segment metadata as well while retaining SDK prewarm/FTS validation, and cover a cold-session legacy index whose name differs from its column.
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
FE Regression Coverage ReportIncrement line coverage |
3 similar comments
FE Regression Coverage ReportIncrement line coverage |
FE Regression Coverage ReportIncrement line coverage |
FE Regression Coverage ReportIncrement line coverage |
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
FE Regression Coverage ReportIncrement line coverage |
What problem does this PR solve?
Related PR: #68775. Related issue: #68692.
Adds synchronous Lance index prewarming to
branch-4.2throughWARM UP SELECT ... PROPERTIES ("read_index_only" = "true"). The selected indexes are warmed on eligible backends against a pinned dataset version and the shared Lance session cache. The implementation includes authorization, deadlines/cancellation, bounded BE execution, and regression coverage; ordinary data warmup remains available.Table names and projection qualifiers are preserved during parsing and resolved together for each execution. Multi-statement
USEand repeated prepared executions therefore use the current session catalog/database, with alias restrictions checked at execution. Original table spelling reaches the catalog lookup without being rewritten by the global case setting. Overqualified table names are rejected instead of silently dropping leading components. Lance-C is pinned to upstream4fafcd64525b274b113208390bbcca86bbbc093b, using the same dependency update as #69332. This includes lance-format/lance-c#97: colliding vector index names resolve through segment field metadata on cold sessions, while segment opening and prewarming remain concurrent. Scalar and full-text prewarming retain the SDK validation path. The local prewarm patch and its combined-patch downloader changes have been removed; only the existing Foyer patch remains, with a C++ test context refreshed for the new upstream revision.Prewarm discovery reads raw user-index names and field IDs from the same pinned query-access snapshot, including legacy scalar indexes without
index_details. It merges physical segments by logical name, excludes internal indexes, and retains discovery bounds; query-planning index discovery remains unchanged.The feature is ported from #68775 at
7ae702f5c500c6dc230053bbb1a8bf9c9a61930f, with the review fixes above.Release note
Support synchronous Lance index prewarming through
WARM UP SELECTwithread_index_only=true.Validation
LanceIndexPrewarmResolutionTest,LanceIndexPrewarmTest,LanceIndexPrewarmAuthTest,WarmupSelectIndexCommandTest,WarmupSelectIndexExecutionTest,LanceMetadataLoaderTest,LancePrewarmIndexDiscoveryTest,LanceIndexInspectionTest,LanceIndexInspectionExecutorTest, andCacheHotspotManagerTableFilterTest.bash thirdparty/test/lance-prefilter-patch-test.sh thirdparty/src, including fresh extraction, idempotence, re-extraction, stale-marker refresh, and patch-failure handling. The obsolete combined-patch marker is no longer used.git diff --checkpassed.run buildall.Check List (For Author)
Check List (For Reviewer who merge this PR)