re20052 commented on PR #66187: URL: https://github.com/apache/doris/pull/66187#issuecomment-6186216442
> Complete static review of PR #66187 at head `3643861cb082c189ee0275ddd33836ce50e4d39c`. The review covered all 12 changed files, their binding and scan consumers, existing inline threads, and one converged round of normal and risk-focused independent review. No new distinct inline finding remained after deduplication and the final changed-file sweep. Three previously reported P1 findings still apply, so I request changes: > > * Existing comment 3670590359: with `cloud_partition_version_cache_ttl_ms <= 0`, the new batch prefetch discards its version vector and the selection loop still makes N synchronous single-partition RPCs under planning locks. > * Existing comment 3670590361: current-transaction writes into a higher partition still have initial committed visible version and cannot enter the single partition chosen before transaction-aware pruning. > * Existing comment 3687666247: the new statement flag disables prepared short-circuit reuse but does not disable SQL result-cache admission or pre-binding lookup; a stale cloud partition choice can be cached past its partition-version TTL. > > **Goal, scope, and tests.** The new non-reserved clause is parsed, propagated through `UnboundRelation`, digested, and bound to an existing OLAP/MTMV partition scan. Ordinary RANGE, LIST, unpartitioned, join, CTE, and aggregation cases follow the stated published-version and LIST DEFAULT fallback contracts. The current code does not fully achieve correct selection in transaction-load or cache scenarios above. The change is localized to FE parser, planner, catalog selection, and tests; no broader source change was needed to assess it. I inspected the parser test, regression suite, and checked-in `.out`; its ordinary and negative expectations match the current stated contract. Cloud TTL, current-transaction, and SQL-cache regressions are missing. The review environment forbids builds and tests, so none were run; checked-in results and PR claims are not independent execution evidence. > > **Concurrency and lifecycle.** Binding runs while FE table metadata is protected during planning. The cloud version path uses version-cache locking and metadata RPCs; at zero TTL the loop still performs serial network calls while planning holds table locks. I found no new thread entry, cross-translation-unit static initialization, lock-order change, or resource-release path. `StatementContext` carries the modifier through a statement, and the point-query rewrite now excludes it, but the separately managed SQL result cache can preserve its data-dependent decision. Privilege checking now sees a catalog scan before later empty pruning; the old empty-fold authorization bypass is fixed. > > **Conditions, parallel paths, and compatibility.** The explicit condition `visibleVersion > 1` means a published delete or zero-row load may yield an empty result; the PR now states that limitation. LIST DEFAULT is deliberately a last-resort fallback. CTE aliases, external/view targets, and time-travel combinations are rejected; the OLAP/MV system-table early-return route is unreachable because those tables expose no supported `$` suffixes. Index, tablet, and scan-parameter validation continues after a null selection. The parser keyword is non-reserved, and this FE-only change uses the existing scan plan; no new FE-to-BE field, storage format, persistent metadata, write path, EditLog, or failover behavior was added. No new configuration item was added; the existing cloud TTL setting exposes the performance defect. > > **Performance, observability, and remaining threads.** Apart from the carried cloud RPC problem, I found no substantiated new CPU or memory issue. Existing warning/log/profile paths provide diagnostics; I found no separate observability defect. Earlier comments about digest, prepared point plans, CTE/snapshot validation, empty-path validation and privilege checks, and final test cleanup are fixed at this head. The delete and LIST DEFAULT comments describe behavior now explicitly accepted by the PR contract. No additional user review focus was provided. This review is complete as a static inspection, with the three existing P1s left as blockers. > > Existing P0/P1 findings confirmed for this head: [#66187 (comment)](https://github.com/apache/doris/pull/66187#discussion_r3670590359), [#66187 (comment)](https://github.com/apache/doris/pull/66187#discussion_r3670590361), [#66187 (comment)](https://github.com/apache/doris/pull/66187#discussion_r3687666247) I have reviewed all of the AI review comments on this PR and addressed the issues that require fixes. I do not believe the remaining three comments need to be handled in this PR: The first concerns additional RPCs and planning overhead in Cloud mode when the partition-version cache TTL is set to zero. It does not affect query correctness and is a lower-priority performance issue, so I do not plan to address it in this PR. The second concerns a semantic boundary: whether partitions written within the current transaction but not yet committed should participate in the selection. max_visible_partition() currently selects partitions based only on published visibleVersion values. Therefore, partitions containing only uncommitted writes from the current transaction are intentionally excluded. The third concerns SQL Cache potentially retaining an incorrect partition selection under a specific combination of Cloud mode, TTL settings, and execution timing. This issue was not introduced by this PR. The existing empty-partition pruning path for ordinary queries is exposed to the same risk. It should therefore be tracked and addressed separately rather than treated as a blocker for this PR. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
