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).
| long elapsedMs = ctx.getStartTime() > 0 ? Math.max(0, System.currentTimeMillis() - ctx.getStartTime()) : 0; | ||
| long remainingMs = TimeUnit.SECONDS.toMillis(ctx.getQueryTimeoutS()) - elapsedMs; | ||
| long deadline = started + TimeUnit.MILLISECONDS.toNanos(Math.max(0, remainingMs)); | ||
| tableName.analyze(ctx); |
There was a problem hiding this comment.
[P2] Validate projection qualifiers against the execution-time table
With current database A and indexed items tables in A and B, send one multi-statement request: USE B; WARM UP SELECT A.items.embedding FROM items PROPERTIES (read_index_only=true). ConnectProcessor parses the entire request before executing USE, so indexWarmupColumns accepts the A.items qualifier against database A and then retains only embedding. This later analyze(ctx) resolves FROM items against B, so the command succeeds and warms B despite the explicit A-qualified projection. Conversely, the valid B-qualified form is rejected during parsing. Resolve the table and validate its projection qualifiers together at execution, preserving the qualifier constraint until then. This differs from the existing direct-qualification thread: it requires session context to change between parsing and execution, including PREPARE/EXECUTE.
| UnboundBlackholeSink<?> sink = new UnboundBlackholeSink<>(project, | ||
| new UnboundBlackholeSinkContext(true)); | ||
| return new WarmupSelectCommand(sink, | ||
| new TableNameInfo(visitMultipartIdentifier(ctx.warmUpSingleTableRef().multipartIdentifier())), |
There was a problem hiding this comment.
[P2] Preserve table spelling until the catalog applies its case policy
With global lower_case_table_names=1, create a Lance catalog lake with its supported lower_case_table_names=0 override and a table named Items. WARM UP SELECT * FROM lake.db.Items PROPERTIES (read_index_only=true) constructs this TableNameInfo, whose list constructor immediately lowercases the table using the global setting. The later lookup honors the catalog's case-sensitive policy but receives items, so warmup fails if only Items exists, or silently warms the different items table if both exist. Ordinary SELECT and data warmup retain the original UnboundRelation spelling and apply the catalog policy. Preserve the original identifier until the selected catalog resolves it, rather than applying the global case setting here.
2 participants
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.Qualified columns and stars resolve the current catalog/database when omitted by
FROM, while table aliases retain their normal restrictions. A Lance-C patch opens colliding vector index names using the field recorded in segment metadata, so a cold session can warm anembeddingindex namedideven when a scalaridcolumn exists. Scalar and full-text prewarming retain the SDK validation path. The third-party downloader fingerprints the complete patch set and refreshes stale or partially patched source caches.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
LanceIndexPrewarmAuthTest,LanceIndexPrewarmTest,LanceIndexInspectionExecutorTest,WarmupSelectIndexCommandTest,WarmupSelectIndexExecutionTest, andCacheHotspotManagerTableFilterTest. The new qualifier reproducer failed before the fix and passed afterward.cargo test --locked --test c_api_test test_prewarm_indexpassed all 5 tests. The new cold-session IVF_FLAT naming-collision test failed before the fix, then passed and verified zero partition reloads for both segments after warming. Coverage also includes scalar/FTS naming collisions, missing indexes, repeated warmup, shared-session reuse, and fixed snapshots.git diff --checkpassed.libsimdutf.a. CI is requested throughrun buildall.Check List (For Author)
Check List (For Reviewer who merge this PR)