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.
COMPLETE static review of PR #68775 at head 60a84c5. Four new P2 findings are inline; no existing P0/P1 inline thread applies. No additional user focus was provided.
Goal and scope: WARM UP INDEX resolves one Lance table snapshot and its storage options in FE, checks the named logical index, and sends bounded batches of RPCs to eligible BEs. The prewarm uses the query-wide Lance session and verifies the opened version; backend selection covers the ordinary external-scan candidates. The change is localized to command parsing/execution, the FE/BE RPC, and the Lance session manager. The current implementation has the prepared-client and cancellation/deadline defects described inline.
Concurrency and lifecycle: FE caps in-flight RPCs at eight and polls cancellation; BE runs SDK work on its heavy-work pool under one process-wide prewarm mutex. Dataset handles and RPC closures have scoped cleanup, and the session singleton avoids cross-translation-unit initialization dependencies. Prepared command reuse leaves cancellation state stale (finding 2), and active BE SDK work can outlive an expired RPC while holding a worker and mutex (finding 3). FE metadata IO can outlive the statement deadline before dispatch (finding 4). No additional lock-order deadlock was substantiated.
Configuration, compatibility, and parallel paths: No new dynamic configuration, on-disk format, transaction, EditLog, or data-write path is introduced. The new protobuf request carries URI, fixed version, index name, storage options and timeout; the response version is required before FE accepts success. An old BE lacking the method fails the RPC instead of returning false success. Query and prewarm paths use the same session, snapshot inputs, and external-backend eligibility. Index-cache residency remains evictable; a successful prewarm is not a permanent pin.
Conditions, security, observability, and performance: ADMIN and table SELECT are checked before external metadata IO, cloud compute-group USAGE is checked before target selection, and the BE rejects embedded NULs. Provider errors are redacted from the new RPC response. Existing session cache metrics expose capacity, usage, entries, hits and misses; the tests do not directly establish cold-to-warm query benefit. Fanout is bounded, but the unbounded active SDK call is a worker/lock availability risk (finding 3). Backtick-quoted identifiers are normalized by NereidsParser PostProcessor before lookup, so that initial concern was dismissed.
Testing and results: I inspected the new FE authorization, fanout, cancellation and parser tests, the BE fixed-snapshot/shared-session test, and the regression suite's success, error, compute-group and privilege cases. Prepared binary protocol, prepared retry after KILL QUERY, slow FE metadata IO, and expired BE SDK work lack discriminating coverage; the regression runs a query before prewarm and does not measure cold-to-warm cache behavior. No .out result file changed. Builds and tests were prohibited by the review prompt, so all conclusions are from static inspection, not executed validation. The final changed-file sweep and two convergence rounds found no unresolved candidate beyond the four inline issues.
| } | ||
|
|
||
| @Override | ||
| public void run(ConnectContext ctx, StmtExecutor executor) throws Exception { |
There was a problem hiding this comment.
[P2] Expose the prewarm result columns to prepared clients. COM_STMT_PREPARE wraps this command in PrepareCommand, which reads Command.getResultSetMetaData(); because this class inherits the empty default, the prepare response advertises zero columns. COM_STMT_EXECUTE then runs this method and sends the five columns in LanceIndexPrewarm.RESULT_META, so binary prepared clients receive a result schema different from the one they prepared. Override the metadata hook with the same five columns and cover the binary prepare/execute path.
| LanceIndexPrewarm.run(ctx, executor, table, indexName, computeGroup, () -> cancelled || ctx.isKilled()); | ||
| } | ||
|
|
||
| public void cancel() { |
There was a problem hiding this comment.
[P2] Keep cancellation state scoped to one execution. ExecuteCommand.run reuses the WarmUpIndexCommand stored in PrepareCommand for each COM_STMT_EXECUTE; after KILL QUERY calls this setter, cancelled stays true on that retained object. A subsequent execute on the same live connection reaches LanceIndexPrewarm.checkActive and immediately fails as cancelled. Reset or replace per-execution state without losing cancellation of the active run.
| options.push_back(value.c_str()); | ||
| } | ||
| options.push_back(nullptr); | ||
| RETURN_IF_ERROR(format::lance::LanceSessionManager::instance().prewarm_index( |
There was a problem hiding this comment.
[P2] Bound prewarm work after it leaves the RPC queue. timeout_ms is checked only against queue time before this synchronous SDK call. If object-store open or index prewarm runs beyond the FE deadline (or the user cancels), the FE returns but this heavy-pool worker and LanceSessionManager's process-wide prewarm mutex remain occupied; every retry on this BE is rejected until the old call finishes. Propagate a deadline/cancellation to the SDK work or isolate and bound abandoned work so a timed-out request cannot monopolize this path.
| checkActive(deadline, cancelled); | ||
| // This is the same snapshot/access path used by queries, including REST-vended credentials. | ||
| // Never independently resolve latest, index segments, or credentials on each backend. | ||
| LanceTableMetadata metadata = ((LanceExternalTable) table).loadMetadata(); |
There was a problem hiding this comment.
[P2] Include metadata loading in the command's timeout and cancellation path. loadMetadata() synchronously resolves Lance namespace access and opens/reads the remote dataset, but the next checkActive is only after that call returns. With a slow object store, KILL QUERY or query_timeout leaves the FE command thread blocked well past its deadline before any BE RPC is sent. Run this phase with a bounded, cancellable deadline and stop before dispatch if it expires.
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
|
run buildall |
|
/review |
There was a problem hiding this comment.
COMPLETE static review of PR #68775 at head 57d3885. I inspected all 22 changed paths, the FE-to-BE call chain, both normal review passes, the separate risk-focused passes, and the final changed-file/candidate sweep. No new substantiated inline findings remain. The user supplied no additional focus points.
Critical checkpoints:
- Goal and proof:
WARM UP INDEXresolves an authorized Lance table and named index, pins a positive dataset version and access options, sends bounded prewarm RPCs to every eligible BE, requires each BE's matching-version acknowledgement, and returns a five-column result. FE, BE, parser, prepared-statement and regression tests are present; this review inspected them but did not run them. - Scope: the parser/command, FE coordinator, protobuf RPC, BE service/session, and focused tests are the required cross-layer changes. No unrelated source change was identified.
- Concurrency: FE metadata loading uses a bounded pool and polls the statement deadline/cancellation; fanout is capped at eight. BE uses a dedicated one-worker/one-queue pool and a process-wide prewarm mutex. No nested-lock cycle was found. Active SDK I/O can still hold that worker and mutex after caller timeout, as already reported in existing P2 comment 4216115274.
- Lifecycle and memory: the FE metadata worker retains its native resources until completion; BE owns the opened dataset with RAII, and the shared Lance session is function-local initialized. No cross-translation-unit initialization dependency, ownership leak, or concrete missing-MemTracker failure was substantiated. The existing configured cache capacity bounds cache storage.
- Configuration and compatibility: no new configuration or persistent format was added. The new RPC carries URI, version, index name, storage options and deadline; FE rejects missing/wrong-version acknowledgements, so older BEs fail explicitly rather than report success. No dynamic-config requirement was introduced.
- Parallel paths and conditions: prewarm and ordinary/vector/full-text scan readers use the same pinned URI/version/options and BE session; scan backend filtering matches the prewarm policy. Branch scans use a distinct URI. Direct schema discovery does not consume the warmed scan cache. Positive-version/index-existence, embedded-NUL, timeout and failure checks were traced; no distinct incorrect parallel path was found.
- Tests and results: added tests cover auth, backend fanout, cancellation, prepared metadata/execution, SDK cache reuse, service queue admission and an external regression. Their assertions and API signatures were inspected; no build, test, CI result, or generated output was executed or independently verified under the review-only instruction. A provisional test compilation concern was dismissed because
doris_be_testuses-fno-access-control. - Observability, transactions and writes: response failures identify the index/version/backend without exposing vended credentials; existing Lance session cache metrics cover cache activity. This command changes only cache state, with no transaction, EditLog, durable write, or data-visibility path.
- Performance and other risks: bounded fanout and admission limit concurrent work; the active BE SDK deadline limitation is the still-applicable existing P2 thread. No additional performance or correctness regression was substantiated.
Existing P2 comments 4216115261 (prepared schema), 4216115269 (per-execution cancellation), and 4216115282 (FE metadata wait) appear addressed at this head. Comment 4216115274 remains relevant to active BE SDK work, although the new dedicated pool reduces impact on shared RPC workers. None is P0/P1, so existing_blocking_comment_ids is empty. No duplicate comment was posted.
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
FE Regression Coverage ReportIncrement line coverage |
FE UT Coverage ReportIncrement line coverage |
|
run buildall |
|
/review |
There was a problem hiding this comment.
Static review complete for PR #68775 at head 5b91e10b1d1ac089999b05524ac82b814b2ed263. I reviewed all 23 changed paths in two convergence rounds, including the FE command and metadata path, BE session/RPC path, protobuf, tests, and both regression suites. One new P2 finding is inline below. No additional user review focus was provided.
Critical checkpoints:
- Goal and scope: The command authorizes Lance index prewarm, pins a dataset version, fans out to eligible BEs, and requires each BE to acknowledge that version. It does not fully cover listed legacy indexes because the FE checks a segment-planning list as though it were the complete logical-name list (inline finding). The changes are otherwise focused on this command and its tests; the Iceberg test edit restores its prior session timeout.
- Concurrency and lifecycle: FE uses a bounded metadata pool and per-
StmtExecutorvolatile cancellation state; BE uses a dedicated one-worker/one-queue pool and a prewarm mutex. I found no new lock-order or static-initialization defect. Dataset handles are closed after the SDK call, and the process-wide Lance session is reused by scans. An in-flight BE SDK call can still outlive the FE deadline; that concern is already in thread 4216115274. - Configuration and compatibility: No new configuration or persistent format is introduced. The FE passes URI, fixed version, index name, storage options, and timeout in the new RPC and accepts success only with OK status and the matching version. An older BE without the RPC fails explicitly during a rolling upgrade. The ordinary Lance scan path and prewarm use the same eligible backend policy, metadata identity, and shared BE session.
- Parallel and conditional paths: I checked normal and search scans, batch and nonbatch placement, cloud and non-cloud compute groups, quoted identifiers, prepared execution, cancellation, missing indexes, and BE rejection. The legacy-name validation is the only new substantiated failure. ADMIN, table SELECT, and cloud group USAGE checks precede remote metadata or dispatch, and provider/RPC errors are sanitized.
- Tests and results: The added BE and FE unit tests and Lance regression cover pinned versions, cache/session reuse, fanout, errors, auth, prepared metadata, and retries; the Iceberg regression restores its original query timeout. They do not cover a legacy index omitted from FE segment metadata. This was static inspection only: the review instructions prohibit builds and tests, so none were run and no test result is claimed.
- Observability and performance: Existing Lance cache metrics and explicit status/acknowledgement paths remain available. Fanout and worker admission are bounded. No new performance or observability defect was substantiated beyond the already reported BE SDK timeout behavior.
- Persistence and writes: This command warms cache state and adds no transaction, EditLog, durable metadata, or table-data write path; transaction atomicity and failover checkpoints are not applicable.
Existing inline P2 threads were not reposted: 4216115261 (prepared result metadata) and 4216115269 (per-execution cancellation) are addressed in this head. Thread 4216115282's FE wait is now bounded by the metadata executor, although native work may continue in its bounded pool. Thread 4216115274's in-flight BE SDK timeout concern remains, with its impact isolated by the new dedicated pool. There are no existing P0/P1 inline findings to carry as blocking IDs. Review completion refers to this static review and submitted finding, not to a claim that all code concerns are fixed.
|
run buildall |
|
/review |
There was a problem hiding this comment.
Review execution complete; convergence incomplete. The third and maximum permitted review round produced a new, independently verified finding, so there was no clean convergence round. I recommend changes for one P1 test-build blocker and three P2 cloud regression-test defects. The review was static only: no build or test was run.
Goal and scope: The change adds synchronous WARM UP INDEX SQL, resolving a Lance index at a pinned dataset version and sending the URI, storage options, version, logical index name, and deadline from FE to selected BEs. BE opens the same snapshot, prewarms through the shared Lance session, and acknowledges the result. The implementation is focused on the command, metadata, RPC, and BE service, with additive protocol fields and no storage-format change. The supplied FE, BE, and regression tests cover ordinary, prepared, auth, timeout, and result paths in source, but M1 prevents the new BE service test from compiling and M2-M4 make cloud regression assertions unreliable.
Concurrency and lifecycle: The FE command owns per-execution cancellation state and uses bounded metadata/RPC waits; the BE service admits a bounded prewarm worker and queue, with closure completion on inspected success and failure paths. The session prewarm mutex serializes native work. No separate lock-order deadlock or cross-translation-unit static-initialization dependency was found. Native metadata and SDK work may outlive the caller deadline; those behaviors are already covered by existing inline threads 4216115282 and 4216115274 and are not reposted.
Configuration, compatibility, and parallel paths: No new configuration item is introduced. The protobuf RPC is additive; an older BE lacking the method returns an explicit RPC failure. Direct and prepared execution, cloud and shared-nothing backend selection, and the ordinary Lance scan session were traced. The FE-to-BE request carries the values needed by the BE's pinned snapshot. Quoted identifiers are normalized before command construction. Index-existence, positive-version, timeout, backend status, and RPC failure checks report errors rather than silently claiming success. The still-applicable legacy logical-index gap is already reported in P2 thread 4218850983.
Correctness, tests, and operations: This command populates process cache and does not write table data, rowsets, transactions, EditLog state, or durable metadata; write atomicity, MoW delete bitmaps, and failover replay are therefore outside this change. The result reports version, backend count, and elapsed time, and the inspected cache/service paths provide existing diagnostics. Bounded dispatch limits admission, while the existing SDK timeout thread covers long-running native work. Static inspection found no other substantiated performance, memory-ownership, or observability defect. Test results and CI claims were not independently validated.
Existing-thread disposition and focus: The two earlier prepared-client P2 issues (4216115261, 4216115269) are fixed on this head; the two timeout P2 issues and the legacy-index P2 issue remain covered by those threads. There are no existing P0/P1 blockers to carry. The user supplied no additional review focus. All 23 changed paths and all four new inline findings were swept and deduplicated against live PR comments.
| auto flight = std::exchange(config::brpc_arrow_flight_work_pool_threads, 1); | ||
| auto* env = ExecEnv::GetInstance(); | ||
| previous_load_mgr = std::move(env->_load_stream_mgr); | ||
| env->_load_stream_mgr = std::make_unique<LoadStreamMgr>(1); |
There was a problem hiding this comment.
[P1] Make the new BE service test compile. This fixture directly moves and assigns private ExecEnv::_load_stream_mgr here and later accesses protected PInternalService pools, even though it only derives from testing::Test. The file is globbed into doris_be_test, so that target fails compilation before these tests can run. Use public test hooks or suitable friend/test subclass access for both objects.
| if (isCloudMode()) { | ||
| def groups = sql "SHOW CLUSTERS" | ||
| assertFalse(groups.isEmpty()) | ||
| def explicit = sql "${statement} WITH COMPUTE GROUP `${groups[0][0]}`" |
There was a problem hiding this comment.
[P2] Choose an available compute group for this success assertion. SHOW CLUSTERS is sorted by name and includes groups with zero or unavailable backends, so groups[0][0] can differ from the healthy current group used by the preceding successful prewarm. In that valid multi-group state, selectBackends() throws No available backends here and the assertion fails. Select the current group from is_current, or choose a group with an eligible BE.
|
|
||
| // Fail one target only: successful prewarm on other BEs must not hide the failure. | ||
| def backends = sql_return_maparray "SHOW BACKENDS" | ||
| def target = backends.find { |
There was a problem hiding this comment.
[P2] Choose the injected backend from the prewarm-eligible set. This filter accepts an alive BE with SystemDecommissioned=false even while it is decommissioning, but needLoadAvailable() excludes decommissioning BEs. If find picks that BE, the command can warm another BE and succeed without hitting the debug point, so the failure assertion fails independently of the cloud tag-key fix. Use the same eligibility policy as LanceIndexPrewarm.selectBackends().
| assertNotNull(target) | ||
| String failureStatement = statement | ||
| if (isCloudMode()) { | ||
| String targetGroup = new JsonSlurper().parseText(target.Tag).cloud_cluster_name |
There was a problem hiding this comment.
[P2] Read the displayed compute group tag in the cloud failure test. SHOW BACKENDS exposes this field as compute_group_name (Backend.getTagMapString() renames it), so cloud_cluster_name is null here. The test sends a compute-group clause with a null name and fails before reaching the injected BE prewarm error; use compute_group_name.
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
FE Regression Coverage ReportIncrement line coverage |
|
run buildall |
|
/review |
Gabriel39
changed the title
There was a problem hiding this comment.
Complete static review of PR #68775 at ebb50b0. All 23 changed files were covered; two review rounds converged. One new P2 is inline. No builds or tests were run, as instructed.
Goal and coverage: SETTINGS(read_index_only=true) parses into the existing WARM UP SELECT command, checks ADMIN and SELECT, resolves a pinned Lance snapshot, and sends version-checked prewarm RPCs to eligible BEs using the shared Lance session cache. This mostly implements the feature, but the new prepared-statement table-resolution finding and the already-reported legacy logical-index omission prevent full correctness. The added FE/BE unit and regression cases were inspected, not executed; the binary prepare regression uses a fully qualified table and cannot catch the new issue.
Design, parallel paths, and compatibility: The changed scope is focused on parser/FE coordination, BE service/session, protocol, and tests. Ordinary data-cache warmup remains on its prior path. Cloud and non-cloud group selection matches the external-scan eligibility policy; all candidate BEs are covered. The new protobuf RPC is additive, and an old BE or missing/wrong snapshot acknowledgement causes an error rather than success. No new configuration, FE-BE session variable, storage format, transaction, persistence, or data-write path is introduced.
Concurrency, lifecycle, and errors: FE metadata work uses a bounded pool and statement deadline; fanout is limited to eight RPCs, with cancellation and per-response status/version checks. BE uses a dedicated one-worker/one-queue pool, serializes synchronous SDK prewarm, closes dataset handles, and returns non-OK status on failure. No separate lock-order or ownership defect was substantiated. The SDK call can still run after the FE deadline and occupy the prewarm path; this remains covered by existing P2 comment 4216115274. Error messages preserve index/version/backend context and redact provider storage details. Existing cache metrics and bounded admission cover the main observability/performance paths; no additional distinct regression was substantiated.
Existing inline context: P2 comment 4218850983 still covers legacy logical indexes omitted from FE segment metadata, and P2 comment 4220410241 still covers a decommissioning BE that the failure-injection regression can select. They were not reposted. Earlier FE metadata timeout, prepared cancellation/result-schema, and cloud group/tag test issues are addressed on this head. The only existing P1, comment 4220410219, claims the new BE test cannot access private/protected members; doris_be_test includes this file and compiles with -fno-access-control, so that claim is not independently confirmed and no existing P0/P1 ID is carried. This conclusion is based on static build configuration; compilation was prohibited.
User focus: no additional focus was provided. Review status: complete after the required normal and risk-focused passes, validation, deduplication, and final changed-file sweep.
| 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] Resolve the table name for each prepared execution. WarmupSelectCommand retains this TableNameInfo, and analyze(ctx) fills its missing catalog/database fields in place. A binary prepared WARM UP SELECT * FROM items SETTINGS (read_index_only=true) executed in Lance database A, then executed again after USE switches to Lance database B, still prewarms A; ordinary unbound relations resolve the current database on each execution. Copy the parsed name parts before analysis so each EXECUTE uses its own session defaults.
FE UT Coverage ReportIncrement line coverage |
|
run buildall |
|
/review |
There was a problem hiding this comment.
Complete static review of PR #68775 at ad820f6 (base 508f636). Three convergence rounds, including normal FE/BE passes and separate risk-focused passes, and a final sweep of all 22 changed files are complete. The three new P2 issues are in the inline comments. No P0/P1 issue is independently confirmed on this head.
Critical checkpoints:
- Goal and proof: The change adds index-only WARM UP SELECT using pinned Lance metadata and prewarms each eligible BE through the shared Lance session. Positive, negative, prepare, cancellation, and regression tests were added, but M1-M3 leave timeout, large-fragment cost, and invalid-name behavior incorrect. This is static inspection only; the prompt forbids builds and tests, so none of the added tests or expected results was independently executed.
- Scope: The grammar, FE command/metadata/RPC, BE service/session, proto, and tests form the feature path. The separate Iceberg Flight timeout restoration was also inspected. No additional scope or abstraction issue was substantiated.
- Concurrency and lifecycle: The FE metadata executor has a bounded worker/queue, the BE uses a dedicated one-worker/one-queue pool, and a try-lock serializes SDK prewarm on the process-wide session. Dataset ownership is closed by RAII; the function-local session avoids cross-translation-unit initialization dependence. No new lock-order or ownership-cycle defect was substantiated. Timed-out native BE work can still outlive the caller, as already raised in discussion 4216115274; the dedicated pool limits interference with other RPC work.
- Configuration and compatibility: No new configuration was introduced. Existing query/insert timeout settings are applied inconsistently (M1). The new optional protobuf request/response and FE client/proxy/BE handler carry URI, storage options, fixed version, and status; an older BE fails rather than silently claiming success. No storage-format or function-symbol change was found.
- Parallel paths and conditions: Legacy data warmup remains on its original execution path; cloud and non-cloud target selection matches the external-scan eligibility policy. The index branch rejects unsupported WHERE/EXPLAIN and projection expressions, but fails to reject an overqualified table name (M3). Prepared result metadata and per-execution cancellation fixes were verified in code.
- Tests and results: FE/BE unit and Lance regression coverage includes success, failure, auth, pinned version, prepare, timeout, and cancellation, but lacks the long-timeout, many-fragment, and four-part-name cases behind M1-M3. No .out file was changed. Test outcomes are unknown because execution was prohibited.
- Observability: The command returns index/version/backend count/elapsed time and uses existing session cache metrics and error statuses; no additional observability defect was substantiated.
- Transactions, persistence, and writes: The new operation only warms caches; it does not commit data, alter visible versions, or write EditLog state. Crash/failover atomicity and MoW delete-bitmap checkpoints are inapplicable here.
- FE/BE state transfer and performance: The RPC path and pinned snapshot acknowledgement were traced end to end. Backend fanout is bounded. M2 is an avoidable O(fragment count) metadata read before cache fill.
Existing-thread disposition: Prepared table-name reuse (4226185666), legacy logical-index omission (4218850983), uninterruptible BE SDK work (4216115274), and regression backend eligibility (4220410241) remain covered by their inline threads and are not reposted. The metadata-wait, prepared-schema/cancellation, and cloud tag/group issues previously raised were addressed on this head. The existing P1 compile claim (4220410219) is not carried as a blocker: be/test/CMakeLists.txt globs this service test into doris_be_test but sets -fno-access-control on that target, so the cited private/protected access is permitted. The round-three virtual compute-group test observation is also covered by 4220410241's requested prewarm-eligible target selection.
User focus: No additional focus was provided.
| List<String> columns, BooleanSupplier cancelled) throws Exception { | ||
| long started = System.nanoTime(); | ||
| long elapsedMs = ctx.getStartTime() > 0 ? Math.max(0, System.currentTimeMillis() - ctx.getStartTime()) : 0; | ||
| long remainingMs = TimeUnit.SECONDS.toMillis(ctx.getQueryTimeoutS()) - elapsedMs; |
There was a problem hiding this comment.
[P2] Use the effective execution timeout for index prewarm. This command extends InsertIntoTableCommand, so StmtExecutor.isSyncLoadKindStmt() makes ConnectContext.checkTimeout() and getExecTimeoutS() allow max(insert_timeout, query_timeout) (defaults: 4 h versus 15 min). This separate deadline uses only getQueryTimeoutS(), so a valid index prewarm that takes over 15 minutes fails while the statement still has hours remaining. Derive the deadline from ctx.getExecTimeoutS() or classify this mode consistently as a query.
| } | ||
| checkActive(deadline, cancelled); | ||
| // Use the query access path, including REST-vended credentials, exactly once. | ||
| return ((LanceExternalTable) table).loadMetadata(); |
There was a problem hiding this comment.
[P2] Avoid loading every data fragment for index-only prewarm. loadMetadata() uses the normal scan metadata scope, and LanceMetadataLoader.read() calls dataset.getFragments() and creates one LanceFragmentInfo per fragment before it reads indexes. This command never uses the fragment list, so a table with many small fragments spends O(fragment count) JNI work and FE heap under the prewarm deadline before any BE cache fill. Read the pinned schema/index/access metadata without materializing scan fragments for this command.
| 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] Reject overqualified table names before constructing TableNameInfo. multipartIdentifier accepts four or more components, but TableNameInfo(List) silently keeps only the last three. For WARM UP SELECT * FROM ignored.lake.db.items PROPERTIES (read_index_only=true), this path prewarms lake.db.items and reports success for that different table, while normal relation binding rejects the four-part name. Validate one to three components before taking the index-only branch.
|
run buildall |
|
/review |
There was a problem hiding this comment.
Completed static review of PR #68775 at 7ae702f5c500c6dc230053bbb1a8bf9c9a61930f. I read the full authoritative diff for all 23 changed files, traced the SQL-to-FE-to-BE prewarm path and its scan-side cache consumer, completed one normal FE pass, one normal BE pass, and a separate risk-focused pass, and resolved every candidate. All three passes concluded NO_NEW_VALUABLE_FINDINGS after deduplication. There are no new inline comments.
Outcome and existing review context. The new command pins one Lance metadata snapshot, maps requested columns to index names, fans out bounded RPCs, and requires each backend to acknowledge the fixed version before returning rows. Existing inline review comment 4220410219 remains a P1 blocker: the added BE service test accesses private ExecEnv::_load_stream_mgr and protected PInternalService pools from a testing::Test fixture, so the BE test target cannot compile as written. I independently checked the access declarations and current test lines and have carried the existing comment ID instead of reposting it. Existing P2 threads still cover overqualified table-name truncation, full fragment loading, the query-versus-effective execution timeout, retained prepared table defaults, omitted legacy index names, unbounded SDK prewarm after dequeue, and the regression debug-target filter. The virtual cloud compute-group case found in this review is at that same filter and covered by its existing policy-match thread, so it is also not reposted. Earlier threads about metadata caller timeout, prepared cancellation/result metadata, and the old regression group/tag choices appear addressed on this head.
Critical checkpoints. Goal and tests: the intended index-only mode and data-mode separation are represented by parser, FE/BE unit, and Lance regression changes, including negative inputs and prepared execution; those files were inspected, not executed here. Focus and scope: no additional user focus was supplied; changes are concentrated on this command/RPC path, with one independent Iceberg timeout-test repair and removal of timing gates from table-filter tests. Concurrency: the FE metadata executor and BE one-worker/one-queue pool bound admission, FE cancellation stops waiting, and dataset cleanup is owned by the worker; the BE SDK call still holds the process-wide prewarm mutex beyond the FE deadline, as already threaded. No new lock-order cycle was found. Lifecycle: the shared BE session initializes lazily, the prewarm handle closes after each SDK call, and queued expired work is rejected; abandoned synchronous SDK work can still delay later prewarms, as already threaded. Configuration: no new dynamic configuration item was added; the existing Lance cache configuration is process scoped. Compatibility: the optional proto request/response fields carry URI, fixed version, index name, storage options, and timeout; missing or old-BE acknowledgements fail explicitly. No storage-format or function-symbol change was found. Parallel paths and conditions: ordinary data warmup keeps its scan/result behavior; cloud and non-cloud backend policy was compared with external-scan selection, while parser validation rejects unsupported index-only WHERE, EXPLAIN, expressions, and modified stars. Test coverage and results: the changed tests address success, error, cancellation, prepared metadata, BE admission, and cloud failure injection, but the BE fixture compile blocker prevents that new BE suite from running as written. No .out result file changed. This review ran no builds, tests, or product-source edits; author/CI claims are not independent validation. Observability: shared Lance session cache metrics and explicit failure statuses remain available; no new monitoring requirement was substantiated. Transaction, persistence, and writes: this is read/cache prewarm work and adds no committed-data write, EditLog, or durable state path. FE-to-BE state: the fixed snapshot and vended storage options are sent in every RPC and checked on acknowledgement. Performance: RPC fanout and worker queues are bounded, while the already-threaded full-fragment metadata read and post-dequeue SDK timeout remain the concrete concerns. No further distinct correctness, security, memory, or performance defect was substantiated.
Review completion is complete at the pinned head. The separate blocking verdict should follow the still-applicable existing P1 comment.
Existing P0/P1 findings confirmed for this head: #68775 (comment)
FE UT Coverage ReportIncrement line coverage |
FE Regression Coverage ReportIncrement line coverage |
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
2 participants
What problem does this PR solve?
Related issue: #68692 (M2); parent tracker: #66340. Dependency baseline: #68698.
Lance index reads currently populate the BE cache during foreground queries. Extend the existing
WARM UP SELECTinterface to preload index contents into the same shared Lance session before queries run:Omitting the setting or specifying
falsepreserves the existing data-scan/file-cache warmup path and its result schema. The options reuse the existingPROPERTIESgrammar without adding a keyword. Index-only mode reusesWarmupSelectCommandand dispatches prewarm RPCs without executing a table-data scan. It does not require the data-file cache to be enabled. The separateWARM UP INDEXsyntax is removed.The FE validates ADMIN and table SELECT privileges, plus current compute-group USAGE in cloud mode. It resolves the normal catalog access path (including vended credentials), pins one dataset snapshot and eligible BE set, and maps projected column names to Lance field IDs.
*selects all logical indexes; a column list selects associated indexes without repeating logical indexes that have multiple segments. Unknown/ambiguous columns, a selection with no indexes, invalid properties, and unsupported catalogs fail explicitly. Index-only mode rejects WHERE and EXPLAIN, as well as modified/mixed star projections, rather than ignoring their semantics.All selected indexes share one SQL deadline. The FE warms indexes sequentially with at most eight BE RPCs in flight, returning success only after every selected index completes on every target. The result has one row per index: table, index, dataset version, backend count, and elapsed milliseconds for that index's RPC phase. Binary PREPARE advertises the same five columns as EXECUTE. Data mode retains its six-column scan/cache statistics. Use the current session's compute group; select another group in the session before prewarming it.
The BE isolates synchronous SDK calls in a dedicated bounded worker pool and opens the pinned dataset through
LanceSessionManager. Failed or older BEs, missing acknowledgements, unsupported indexes, and storage errors fail explicitly without exposing provider credentials. SQL timeout and KILL/connection cancellation stop waiting, including during metadata reads. Prepared-statement retries do not inherit cancellation. Cancellation does not guarantee immediate interruption of SDK IO or undo completed cache fills; cached entries remain evictable.Release note
Add
PROPERTIES ("read_index_only" = "true")toWARM UP SELECTfor synchronous Lance index prewarm on eligible query backends, selecting all indexes or indexes associated with projected columns.Validation
-Xint) before the change and passed with the same JVM configuration after it. All 109 related FE tests passed (0 failures, 0 skips), including the complete table-filter suite and the Lance prewarm tests; FE Checkstyle also passed.test_lance_index_prewarmregression suite on an isolated local FE/BE cluster backed by MinIO: 1 suite passed, 0 failures or skips. The FE used the newly compiled implementation; the unchanged BE used the PR's compiled artifact. The synthetic fixture contained both an IVF_FLAT vector index and a BTREE scalar index.git diff --checkand clang-format 16 checks for all C/C++ files changed by this PR passed.libsimdutf.a. Cloud execution was covered by FE unit tests, not a local cloud cluster. The broader external regression pipeline remains CI validation.Companion bilingual documentation: apache/doris-website#4196. The broader supported-format/catalog matrix and lifecycle/performance verification remain tracked in #68692 M3. This PR does not close that issue.
Check List (For Author)
Check List (For Reviewer who merge this PR)