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.
Reviewed all 61 changed paths at a45d9bf using the supplied aggregate diff. Two P2 issues warrant fixes: eager sorting can reject valid mixed-spec Iceberg partition displays, and weighted admission prevents large partition views from being cached. Both have inline evidence and concrete triggers. No P0/P1 finding was substantiated; there were no existing inline threads to duplicate or carry as blockers.
The reviewed flow is relation discovery -> statement snapshot/schema pin -> deferred native planning or partition-view conversion -> scan request -> per-reference SQL-cache recording and revalidation.
Critical checkpoint conclusions:
- Goal and proof: native planning and immutable metadata generations reduce repeated partition/existence work while retaining selected read versions. Added component tests support the main flow; the two inline cases remain gaps.
- Scope and clarity: changes stay within FE connector, MVCC, planner and result-cache plumbing; the binlog test change is a mechanical dependency-key adaptation.
- Concurrency and locks: request planning, split workers and refresh/loaders share concurrent statement memos, generation-aware cache publication and retained leases. No additional lock-order or deadlock issue was substantiated.
- Lifecycle: frozen metadata and resource leases remain statement-owned across refresh; deferred loading uses the pinned handle/schema. No premature close, ownership cycle or static initialization problem was substantiated.
- Configuration: enable_iceberg_internal_partition_pruning defaults to false and is captured from session state; later statements observe session changes without restart. Partition-aware consumers retain eager planning.
- Compatibility: default SPI methods preserve other connector behavior; API 18.0 advances to 19.0 with both surface baselines updated. No persisted storage format changes occur.
- Thrift/protobuf: no definitions or field IDs changed.
- Parallel paths: latest, historical, branch/tag, empty/system reads, DDL/CTAS, synchronous/streaming scans, COUNT pushdown, MV compensation and partition rules were traced; unsupported options/incremental cache paths remain excluded.
- Conditional/error paths: native unknown remains distinct from an empty read; newly installed partition rules reject unknown counts. Version acquisition failures disable caching. Eager mixed-spec index construction is the exception covered inline.
- Coverage: reviewed new unit cases for pins, selectors, schema/ref changes, cache identity, refresh and concurrency, plus the regression suite. Mixed-spec sorting and large weighted admission need coverage. Builds and tests were prohibited and were not run.
- Changed expected results: cold/warm rows, branch/tag/main aggregates, moved branch, refresh and recreation outputs agree with the scripted operations. Generation of the supplied output file was not independently rerun.
- Observability: native EXPLAIN identifies unknown counts; existing cache rejection metrics and load-failure logging cover the changed paths. No additional logging/metrics defect was substantiated.
- Transactions and persistence: no EditLog, replay, commit or failover protocol changes occur.
- Writes and crash atomicity: no data-write or transaction-atomicity logic changed; existing DDL paths continue to use writable metadata.
- FE/BE variables: no transmitted field or BE protocol addition; the new session preference affects FE planning.
- Performance and memory: native pruning, view reuse and deferred MV partition unions remove repeated work. The weighted-cache issue defeats reuse under supported memory limits; eager sorting also affects display-only views.
- Other invariants: selected snapshot/schema identity is retained, and no new rowset visibility, delete-bitmap or BE allocation behavior is involved. No additional verified issue remained after the missed-issue sweeps.
User focus: no additional focus points were supplied. Coverage limit: this is a static review; the new integration regression disables SQL cache, so end-to-end cache behavior and the PR's performance/test claims were not independently executed. No changed path or concrete candidate remains unreviewed or unresolved.
Review converged after 2 round(s).
TPC-H: Total hot run time: 28653 ms |
TPC-DS: Total hot run time: 151208 ms |
ClickBench: Total hot run time: 24.01 s |
a45d9bf to
12ba5ca
Compare
|
run buildall |
|
/review |
|
CI failure triage for the rebased head:
Local validation on this repaired head: 210 scoped tests, an overlapping 21-test weighted-cache/runtime pass, and build.sh --fe with Checkstyle passed. New buildall and review runs have been requested; remote CI completion is still pending. |
There was a problem hiding this comment.
Reviewed head 12ba5caef8d92a09526c8172f567d40052d6d5b6 using the supplied aggregate diff and all 67 changed paths. One new P2 regression is reported inline: a cached Iceberg table handle breaks HMS DROP TABLE IF EXISTS after an external drop. No new P0/P1 finding was verified.
The existing weighted partition-view admission report and heterogeneous partition-key sorting report are addressed on this head: the dedicated estimator reaches catalog runtimes, and mixed LIST key shapes retain their maps without constructing an invalid sorted index. Neither is reposted. No additional user focus was supplied.
Critical checkpoint conclusions:
- Goal and evidence: native Iceberg scans can defer engine partition conversion; immutable views/indexes are reused by generation, and SQL-cache dependencies retain each original selector. Focused unit and handle/ref/schema/refresh regressions support those paths; the inline DDL edge remains uncovered.
- Scope and abstraction: changes consistently address metadata reuse, planning, and result-cache identity. Generic engine code uses SPI defaults and source-neutral carriers.
- Concurrency and locks: statement lookup uses ConcurrentHashMap; deferred views are memoized and published after construction; existing cache generation fences and borrower reference counting protect refresh/eviction. No new lock-order defect was verified.
- Lifecycle: statement-frozen metadata and retained schemas feed binding and scans; leases survive cache retirement and release at statement cleanup. No new static-initialization issue was identified.
- Configuration:
enable_iceberg_internal_partition_pruningdefaults to false, propagates through captured session properties, and is observed without restart. MV candidates and partition-aware block rules retain eager metadata. - Compatibility: connector API advances from 19.0 to 20.0 with both recorded baselines and the version assertion updated. No persisted storage format or FE/BE wire change is introduced.
- Thrift/protobuf IDs: no definitions changed.
- Parallel paths: checked latest/historical/branch/tag reads, empty/no-scan cache results, synchronous/streaming scans, system tables, LIST/RANGE views, cache-disabled/offline calls, and DDL. The new issue is in the DDL existence path.
- Conditional semantics: NATIVE is unknown/all, not prune-to-zero; predicates remain on the scan. A partition rule introduced during native planning fails with a retry instead of treating an unknown count as zero.
- Tests and negative cases: inspected selector immutability, expired refs, schema-only changes, recreation, deferred-load concurrency/failure, cache invalidation and 12,000-partition weighted admission. No build or test was run under this static-only contract.
- Changed expected output: counts, sums, branch/tag contents, refresh and recreation results agree with the regression statements. Output generation was not independently executed.
- Observability: native partition counts are explicitly shown as unknown; existing cache metrics and contextual failures remain available. No additional observability defect was verified.
- Transactions, persistence and failover: no transaction protocol or EditLog format changes; caches and pins remain transient. No new persistence gap was verified.
- Data writes and atomicity: write transaction machinery is unchanged; mutation preflight is affected by the inline stale-existence issue.
- FE/BE variable propagation: the new session switch controls FE planning and requires no new BE field.
- Performance: versioned keys avoid partition fingerprints for Iceberg, views share conversions and indexes, partition collection avoids name-set copies/hashing, and MV unions and duplicate relation listings are deferred/eliminated. Benchmark figures were not independently reproduced.
- Additional risk sweep: metadata-generation/schema/ref identity, cache-miss fallback, shared-item readers, and resource cleanup yielded no further verified new issue.
The reviewed execution chain is CollectRelation -> snapshot/schema pin -> native or eager partition selection -> scan request, with original selector + captured version -> SQL-cache revalidation. Findings are based on static control-flow evidence; runtime integration and timing were not executed.
Review converged after 2 round(s).
12ba5ca to
740783d
Compare
|
run buildall |
|
/review |
There was a problem hiding this comment.
Static review of all 68 changed paths at 740783d found one new P2: CREATE TABLE's separate existence probes still consume cached read handles after an external drop. The inline comment describes the concrete cache-expiry ordering and fix direction. No new P0/P1 finding was confirmed.
The three existing P2 threads were independently revalidated as fixed: heterogeneous LIST keys now omit an incompatible sorted index (4317051884); partition views use a scalable entry-specific estimator through catalog runtimes (4317051894); DROP TABLE IF EXISTS uses a live existence preflight (4321436027). The new finding concerns the separate CREATE consumers, not the repaired DROP path. No additional user focus was supplied.
Critical checkpoint conclusions:
- Goal and proof: pinned metadata versions, reused partition projections and deferred native planning implement the stated optimization. Tests cover the principal contracts; CREATE's stale-cache case needs coverage.
- Scope and reuse: changes stay within connector SPI, shared caches and their planning consumers; common selector logic replaces duplicate key construction.
- Concurrency and locks: immutable projections, generation-fenced invalidation, per-pin memoization and existing table leases preserve active readers. No new lock-order cycle or shared-map traversal defect was substantiated.
- Lifecycle: frozen read metadata survives refresh through statement-owned leases; cleanup follows split-pump completion. Prepared executions reset their metadata ownership; no static-initialization dependency was introduced.
- Configuration: the new session boolean defaults to false and is captured per connector session. MV candidates and enabled partition rules retain eager metadata; later partition-rule changes reject unknown counts with a retry.
- Compatibility: connector API advances from 19.0 to 20.0 with both recorded surfaces and the pinned version test updated. Legacy connectors retain default behavior. No persisted format change was found.
- Thrift/protobuf: no changed definitions or field IDs.
- Parallel paths: current, snapshot/time/ref/branch/tag, empty/system tables, LIST/RANGE, native/eager, synchronous/streaming/COUNT, MV/block-rule, legacy freshness and DDL/write routes were traced. CREATE is the identified exception.
- Conditions and errors: native unknown partitions remain distinct from a prune-to-empty result; snapshot/version errors invalidate result-cache reuse, and deferred-load failures do not publish partial views.
- Test coverage: supplied unit tests cover immutable selectors, exact versions, schema/rollback/ref changes, weighted admission, invalidation, concurrent loading, native filters, counts and rule/MV fallback. No builds or tests were run, as required by this review contract.
- Test results: the new regression's expected rows agree with its insert/ref/refresh/recreation sequence and use ordered queries. Output-generation provenance was not verified. That suite disables SQL caching, so it does not itself establish end-to-end cache-hit invalidation.
- Observability: native EXPLAIN exposes unknown counts; eager counts, contextual errors and existing cache metrics remain available. No additional required instrumentation was identified.
- Transactions and persistence: the new caches, selectors and projections are transient; no journal/replay or failover protocol change was introduced.
- Writes and crash behavior: actual DDL uses live catalog operations, while data writes use separate mutable metadata and generation checks. No new atomicity or resource-release defect was confirmed beyond CREATE preflight routing.
- FE/BE values: no new wire variable; existing predicates, snapshot handles and PCache fields carry the behavior.
- Performance: native version lookup avoids partition enumeration, versioned views reuse conversion/index work, and partition collection avoids unnecessary copies/hashes. Published benchmark claims were not rerun.
- Other and final recheck: all candidates and four initial risk focuses have explicit dispositions; the deferred-worker timezone hypothesis lacked a reachable production trigger and was dismissed. Historical cache probes cannot enable an incompatible MV rewrite because selector-bearing scans are rejected by the existing table-query-operator gate.
Review limits: static analysis only; no runtime, benchmark, build or generated-result provenance verification. No unresolved static candidate or uncovered changed path remains. Delivery and convergence are tracked separately; the final convergence result follows the bounded reviewer recheck.
Converged after 2 review rounds: all three reviewers returned NO_NEW_VALUABLE_FINDINGS on the identical frozen comment set; all changed paths, candidates and risk focuses have dispositions. One P2 is delivered inline; there are no confirmed existing P0/P1 blockers.
Review converged after 2 round(s).
740783d to
b44b384
Compare
|
run buildall |
|
/review |
There was a problem hiding this comment.
Reviewed all 68 changed paths and their relevant callers at b44b384. No new substantiated findings.
The four existing P2 concerns are fixed on this head: heterogeneous LIST keys skip invalid sorted indexes (4317051884); partition views use an entry-specific scalable estimator propagated into the catalog runtime (4317051894); DROP uses a live existence preflight (4321436027); both CREATE existence probes use the live API (4321658993). These concerns are not reposted. No existing P0/P1 finding requires a blocking comment ID.
Critical checkpoint conclusions:
| Checkpoint | Conclusion |
|---|---|
| Goal and supporting tests | The code implements generation-specific partition-view reuse, native Iceberg planning and independent SQL-cache dependencies for current/historical/branch/tag references. Added tests target these mechanisms; execution was not independently verified. |
| Scope, clarity and reuse | The changes follow the existing connector SPI, statement-scope and shared-cache infrastructure; selector keys have one implementation. No unrelated production change identified. |
| Data and version correctness | Scalar snapshot, retained schema, partition projection and scan handle describe the same frozen read. SQL-cache identity includes table UUID, snapshot, schema and metadata publication; original selectors remain separate dependencies and revalidate through MVCC resolution. No new visibility defect found. |
| Concurrency and locks | Concurrent scope lookup, memoized complete projection publication, generation-fenced invalidation and existing lease ownership preserve in-flight readers. No new lock-order problem or heavy I/O under a newly added metadata lock found. |
| Lifecycle and static initialization | Statement borrowers survive shared-cache retirement and close after scan work; offline existence probes do not acquire an unowned lease. New selector/version values are immutable. No static initialization change. |
| Configuration and dynamic behavior | enable_iceberg_internal_partition_pruning defaults false and is captured from session properties; later planning observes SET without restart. Existing SQL-cache opt-in remains required. |
| Compatibility | SPI major version and test pin advance to 20.0 and both recorded surfaces reflect the new API. No persisted format change. |
| Thrift, Protobuf and FE-BE propagation | No gensrc definitions, wire fields, plan-node kinds or newly transmitted variables change. |
| Parallel paths and conditions | Reviewed latest/time travel, branch/tag, view/base/system metadata, native/eager, RANGE/LIST, empty/missing, synchronous/streaming/batch and COUNT paths. Native selection remains an unknown universe, retains filters and cannot become a pruned-empty scan. |
| MV and planner interactions | Candidate collection precedes normal pinning; partition-aware MV consumers remain eager. Non-latest references retained after SQL-cache misses are rejected by the actual query MV structural-validity gate. Existing rule stage order and property behavior are preserved. |
| Error behavior | Unreliable cache versions disable caching; revalidation failures invalidate entries. Late partition rules reject unknown counts. CREATE/DROP liveness is separate from cached query handles. No new silent-success path found. |
| Test coverage | Inspected tests for refresh/lease races, deferred loading, weighted admission, mixed specs, selector immutability, cache identity/revalidation, DDL liveness, partition counts and MV bookkeeping, plus the changed regression suites. |
| Changed expected results | The handle-reuse output matches the fixture: latest count/sum 3/3000000003, original branch/tag 2/3, moved branch follows latest, and recreated table contains 42. Output-generation provenance remains unverified. |
| Observability | EXPLAIN identifies unknown native partition counts; contextual metadata errors and existing cache statistics remain available. No additional instrumentation requirement established. |
| Persistence, transactions, writes and failover | No new durable state or transaction protocol; CREATE/DROP retain existing EditLog/invalidation paths. Read pins remain separate from writable metadata. No new crash-related write/ownership issue found. |
| Performance and memory | Certified generation keys and scalar cache versions avoid partition enumeration; maps/indexes share immutable projection objects, fallback content keys remain linear, and per-partition sizing avoids whole-view traversal limits. Reported performance numbers were not reproduced. BE allocation, MoW and delete-bitmap invariants are unaffected. |
No additional user-provided focus was specified. Coverage is complete for the static review; no unresolved candidate remains. Two complete-scope reviewers and the additional risk reviewer all returned NO_NEW_VALUABLE_FINDINGS on the unchanged empty finding set. Review converged after 1 round.
Validation limits: static analysis only, as required. No builds, tests or source modifications were performed; runtime behavior, benchmark claims and regression output-generation provenance were not independently validated.
Review converged after 1 round(s).
FE UT Coverage ReportIncrement line coverage |
There was a problem hiding this comment.
Static review of #69299 at fe11662 found no new actionable defect across all 68 changed files. The four existing P2 concerns were independently revalidated as fixed: CREATE and DROP use live existence checks; heterogeneous display partition keys skip ordered indexes; partition-view memory admission uses its dedicated scalable estimator. No existing P0/P1 finding remains to carry forward, and no duplicate inline comments are posted.
Critical checkpoint conclusions:
- Goal and correctness: ordinary standalone Iceberg scans can defer FE partition conversion; SQL cache dependencies retain each original read selector and selected snapshot/schema/metadata generation. No new data-visibility or wrong-result defect was substantiated by the traced binding, validation and scan paths.
- Scope and reuse: the change is broad but cohesive across FE metadata planning, cache infrastructure and connector SPI. It reuses statement snapshot resolution and generation-aware caches; no unrelated production change was identified.
- Concurrency and locking: query planning, split producers and concurrent refresh were traced. Immutable map/index publication, memoized deferred loading and retained table leases preserve ownership. Existing table-lock ordering remains; no new lock cycle was found.
- Lifecycle: deferred loaders retain the pinned metadata/session/handle within the statement; cache invalidation does not close active readers. Existing statement teardown follows producer cleanup. No new static initialization or resource-cycle issue was found.
- Configuration: enable_iceberg_internal_partition_pruning defaults false and is captured from session properties for each statement. Internal pruning, eligible MV planning and partition metadata rules retain eager views.
- Compatibility: connector API major 21.0 and both recorded SPI baselines are updated together; default methods preserve conservative fallback behavior. No wire or persisted-format change occurs.
- Thrift/protobuf IDs: not applicable; neither directory changes.
- Parallel paths: latest, historical, branch/tag, empty and system-table reads, plus Hive/Paimon/Hudi paths, were inspected. Hive gateway Iceberg tables do not forward the new optional hooks and retain eager/legacy behavior; unsupported historical cache dependencies are conservatively disabled. The optimization should not be assumed to cover that gateway path.
- Conditions and failures: native partition selection remains unknown rather than empty; predicates reach connector planning. Partition-rule installation during planning fails with a retry when counts are unknown. Unreliable SQL identities disable caching, while metadata failures remain failures. No new silent-success path was established.
- Test coverage: inspected unit cases cover selector isolation, schema/rollback/ref movement, refresh and invalidation, weighted admission, deferred loading and failure, native selection/counts, MV/rule gates, DDL and relation listing. The new handle-reuse regression covers mutations and recreation with SQL caching explicitly disabled, so native SQL-cache integration is supported here by unit and static evidence.
- Changed expected results: the added result rows agree with the fixture operations. Test execution and output-generation provenance were not independently verified; no builds or tests were run under this static-only contract.
- Observability: EXPLAIN distinguishes unknown native partition counts; existing cache statistics and metadata error context remain available. No additional material observability defect was identified.
- Transactions and persistence: new projections are transient; existing DDL edit-log success/no-op/failure paths remain. No new replay or master-failover protocol is introduced.
- Writes and atomicity: the DDL changes separate live preflights from cached reads, retaining existing create-race handling, drop cleanup and write transactions. No new crash/atomicity defect was substantiated.
- FE-BE variables: the new switch affects FE planning only; no new transmitted field needs propagation.
- Performance and memory: native planning/version lookup avoids partition enumeration; cached maps and indexes share partition items and use FE cache weight accounting. Eager cold views still pay index-construction CPU/memory even without binary pruning, a recorded tradeoff with no material regression established here. Performance claims were not benchmarked in this review; BE allocation/delete-bitmap invariants are unchanged.
- User focus and remaining scope: no additional focus was supplied. All changed paths, risk items and candidate dispositions were covered in the static review. Remaining validation limits are runtime behavior, benchmark claims and generated-output provenance; there is no unresolved actionable candidate.
Review convergence is assessed separately from delivery. The final payload is checked by all assigned reviewers before trusted-helper submission.
Review converged after 2 round(s).
FE UT Coverage ReportIncrement line coverage |
FE Regression Coverage ReportIncrement line coverage |
TPC-H: Total hot run time: 28317 ms |
TPC-DS: Total hot run time: 150181 ms |
ClickBench: Total hot run time: 24.38 s |
fe11662 to
3c1f1a4
Compare
|
run buildall |
|
/review |
There was a problem hiding this comment.
Static review of all 68 changed paths at 3c1f1a4bd2c5b60ead57859c23e9a1624c6967be. One new P2 finding: eager read acquisition in getTableHandle prevents metadata-only DROP of a REST Iceberg table configured for server-side scan planning. The inline comment gives the concrete trigger and call chain. Review converged after two rounds; no unresolved code candidates or unreviewed changed paths remain.
The four existing P2 threads were independently revalidated: live CREATE/DROP existence checks, scalable partition-view size estimation, and heterogeneous LIST-key index handling are fixed at this head. They are not reposted. No existing P0/P1 finding applies. The supplied user-focus file contains no additional concerns.
The reviewed read flow is:
CollectRelation -> StatementContext.loadSnapshots -> frozen connector table + scalar/schema pin
-> LogicalFileScan -> native file planning OR pinned partition view/index
-> captured SQL-cache version -> original-reference revalidation
The separate DROP flow still resolves a table handle after its live existence check, which introduces the reported read-planning dependency.
| Critical checkpoint | Conclusion |
|---|---|
| Goal and evidence | Generation-specific views, native partition deferral and per-reference SQL-cache versions implement the stated planning optimization. Added unit/regression tests exercise those paths; the new DROP regression remains. |
| Scope and reuse | Changes stay within connector metadata/planning/cache integration. Shared selector canonicalization and SPI defaults avoid connector-specific branches in generic FE code. |
| Concurrency and locks | Concurrent statement lookup, generation-aware cache borrowing and memoized deferred loading preserve publication and ownership. No additional lock-order/deadlock defect found; complete views publish before cache admission. |
| Lifecycle/static initialization | Deferred loaders retain the pinned handle/session/schema; statement leases end after scan cleanup. No reachable use after scope close or new static-initialization cycle found. |
| Configuration | enable_iceberg_internal_partition_pruning defaults false and is captured per statement; changes apply without restart. MV and partition-rule consumers retain eager metadata. |
| Compatibility | Connector API advances from 20.0 to 21.0 with both recorded baselines updated. Other connectors retain default behavior; no persisted storage-format change. |
| Thrift/protobuf ids | No definition changes. |
| Parallel paths | Inspected current/history/branch/tag, empty snapshots, cache enabled/disabled, no-context callers, system tables, view listing, MV, count/streaming and DDL paths. The new finding concerns the DDL path. |
| Conditions and failures | Native unknown partitions remain scan-all, preserve filters and publish ALL_PARTITIONS for compensation. Unsupported cache identities decline caching; late partition rules reject unknown counts. No additional silent-failure defect found. |
| Test coverage | Tests cover metadata/schema/ref changes, rollback/expiry, refresh races, lazy failure/retry, large weighted views and type/timezone keys. A live-scope DROP test using a real server-planning RESTTable is missing. |
| Expected results | New regression outputs match the SQL and fixtures statically, including main/branch/tag counts and table recreation. Execution and output-generation provenance were not verified. |
| Observability | EXPLAIN distinguishes native unknown counts; load/partition errors and rule-retry errors retain context. No additional critical diagnostic gap found. |
| Persistence/failover | No journal payload/replay or transaction-protocol change. DDL retains its existing post-success logging/invalidation paths, subject to the reported failure before dispatch. |
| Writes/atomicity | No data-write transaction algorithm changed; mutable write tables remain separate from frozen reads. CREATE race checks remain live. |
| FE/BE propagation | The new switch controls FE metadata planning; no new BE variable or serialization field needs propagation. |
| Performance/memory | Versioned hits avoid partition enumeration, shared views/key views reduce copying, and weighted admission scales per partition. View misses eagerly build an O(P log P) index; that cost is an explicit tradeoff. Author performance measurements were not reproduced. |
| Other issues | No additional substantiated finding after complete-scope and focused-risk rechecks. |
Validation was static only, as required: no builds, tests, source changes or runtime experiments were performed. Real-catalog interoperability, concurrency execution and performance remain unmeasured in this invocation. These are validation limits, not unresolved review candidates.
Review converged after 2 round(s).
| try { | ||
| // Loading proves existence and pins the same metadata used by schema/snapshot/scan planning. | ||
| // Borrow through the existing generation-aware cache so concurrent cold readers share one load. | ||
| executeAuthenticated(() -> resolveTableForRead(session, handle)); |
There was a problem hiding this comment.
[P2] Keep DROP handle lookup independent of scan planning
For an existing REST Iceberg table whose load response sets scan-planning-mode=server, run DROP TABLE from a normal SQL session with a cold or disabled table cache. PluginDrivenExternalCatalog.dropTable still calls getTableHandle after its live tableExists preflight, so this new load enters resolveTableForRead and throws from IcebergScanPlanning.rejectServerSideScanPlanning before metadata.dropTable can run. The real RESTTable shape is covered by IcebergRestScanPlanningTest. Previously this lookup only checked existence, and the REST DROP implementation only needs the table identifier. Preserve a DDL handle path that does not require local scan support while retaining frozen metadata acquisition for query reads. This differs from the existing DROP IF EXISTS thread: the table exists and the live preflight succeeds here.
There was a problem hiding this comment.
Fixed in 3bde61a. DROP retains its live existence preflight and now requests a DDL handle instead of the scan-planning read handle. Iceberg returns an identifier-only DDL handle after checking live existence, so server-side REST scan planning cannot block DROP or ALTER. Query reads still use the frozen scan-capable handle path. Added a server-planned-table test that reproduces the read-handle rejection and verifies DROP succeeds; the Iceberg (10) and generic DDL routing (90) unit tests pass.
TPC-H: Total hot run time: 28661 ms |
TPC-DS: Total hot run time: 150693 ms |
ClickBench: Total hot run time: 24.31 s |
|
run buildall |
|
/review |
There was a problem hiding this comment.
Static review of PR #69299 at 3bde61a found three new P2 issues, all included inline: a missing SPI surface baseline entry, a metadata-only EXECUTE caller still acquiring a scan-capable handle, and unused partition-index construction when binary-search pruning is disabled. No new P0/P1 finding was established.
Critical checkpoint conclusions:
- Goal and evidence: frozen-generation reuse, native Iceberg planning, and per-reference SQL-cache versions are implemented through the existing metadata/scope infrastructure. The three inline findings identify remaining exceptions. Relevant tests were inspected; none were executed.
- Scope and reuse: the connector-neutral partition projection, selector key, cache runtime and SPI defaults are focused on the stated metadata-planning changes and reuse existing infrastructure.
- Concurrency and locking: inspected cache borrowing/invalidation, immutable publication, memoized deferred loads and query/worker consumers. No additional race, lock-order change or heavy-under-metadata-lock defect was substantiated.
- Lifecycle: statement-owned leases retain frozen metadata/FileIO across REFRESH; views survive independently for existing readers. Scope cleanup and failure/retry cases are covered in inspected unit tests. No new static-initialization issue applies.
- Configuration: enable_iceberg_internal_partition_pruning defaults false and is captured from each session without restart. The existing binary-search switch is ignored during index construction, as reported inline.
- Compatibility: connector API major advances to 21.0; the new inherited DDL method is missing from the metadata surface inventory. No storage-format change was found.
- Thrift/protobuf: no gensrc definitions changed; field-ID compatibility is not affected.
- Parallel paths: traced current/historical/branch/tag dependencies, native/eager planning, synchronous/batch/streaming/count paths and CREATE/DROP/catalog ALTER. The separate EXECUTE adapter remains on the read-handle path, as reported inline.
- Conditional behavior: NATIVE remains an unknown universe rather than an empty selection; required partitions stay scan-all, filters are retained, and collection records ALL_PARTITIONS. Enabled partition rules/MV candidates force eager metadata; a newly installed rule rejects an unknown count with a retry error.
- Test coverage: inspected positive, negative, refresh, schema/ref, selector, cache-disabled, invalidation, weighted-admission, concurrency, native-count and DDL tests. Missing baseline and EXECUTE/disabled-index cases correspond to the inline findings.
- Changed results: the new ordered regression output matches the fixture arithmetic, branch/tag movement and recreation. Runtime results and output-generation provenance were not verified in this static-only review.
- Observability: unknown-native EXPLAIN text is explicit; version/load errors retain table context, changed-rule errors name the rule, and existing cache statistics remain available. No additional diagnostic issue was substantiated.
- Persistence/failover: no journal format or transaction-publication protocol changed. The adapted changelog test changes its dependency-key mock; statement/cache state is transient.
- Writes/atomicity: existing mutation/edit-log/invalidation fences remain in the examined DDL paths. The EXECUTE finding prevents dispatch before mutation; no new data-write atomicity defect was substantiated.
- FE/BE propagation: the new switch controls FE planning and plugin session behavior; no new FE/BE field or alternate serialization path needs updating.
- CPU/memory: immutable maps/key views, generation keys and deferred unions reduce repeated partition work. The new unconditional ordered-index build adds avoidable O(P log P) work and O(P) allocations in the disabled-pruning case reported inline. Large-view estimation now uses the registered per-record estimator.
- Further recheck: rechecked selector immutability, snapshot expiry/ref removal, metadata/schema alignment, retained resources, non-query handle consumers, unknown counts and all changed files. No additional verified issue or unresolved code-coverage question remains.
Existing review disposition: the five supplied P2 instances are fixed at their reported locations: heterogeneous display keys skip sorting; partition-view estimates reach catalog cache runtimes; CREATE and DROP use live existence checks; DROP/catalog ALTER use identifier-only DDL handles. The EXECUTE inline finding is a distinct remaining caller. No existing P0/P1 comment requires carrying forward. There was no additional user-provided review focus.
Review converged in round 2 after complete assigned coverage, explicit disposition of all candidates/risks, and unchanged-payload confirmation by every reviewer. Validation limits: static inspection only; no builds, tests, runtime probes, performance measurements or source modifications were performed.
Review converged after 2 round(s).
| * require scan-capable metadata may return an identifier-only handle here. Other connectors retain their | ||
| * existing handle resolution behavior. | ||
| */ | ||
| default Optional<ConnectorTableHandle> getTableHandleForDdl( |
There was a problem hiding this comment.
[P2] Record the new DDL method in the SPI baseline
Adding this inherited default also adds getTableHandleForDdl(org.apache.doris.connector.spi.ConnectorSession,java.lang.String,java.lang.String) to ConnectorMetadata.class.getMethods(), but connector-metadata-methods.txt still omits it. ConnectorMetadataSurfaceTest.methodSurfaceMatchesRecordedBaseline compares that complete reflected set with the recorded file, so the SPI unit suite fails at this head. Add the new signature to the baseline alongside this API change.
There was a problem hiding this comment.
Fixed in 068d3c7. Added getTableHandleForDdl to connector-metadata-methods.txt and advanced the connector plugin API major to 22.0, including the version assertion. The separate connector-plugin-surface.txt renderer has no ConnectorMetadata entry to change. Both SPI surface test classes pass (6 tests total), and the full FE build including Checkstyle passes.
| try { | ||
| // Loading proves existence and pins the same metadata used by schema/snapshot/scan planning. | ||
| // Borrow through the existing generation-aware cache so concurrent cold readers share one load. | ||
| executeAuthenticated(() -> resolveTableForRead(session, handle)); |
There was a problem hiding this comment.
[P2] Preserve metadata-only EXECUTE actions on server-planned tables
With a cold or disabled table cache, ALTER TABLE t EXECUTE rollback_to_snapshot('snapshot_id'='<ancestor>') still enters this new read-table load through ConnectorExecuteAction.execute's getTableHandle call. A REST table using server-side scan planning is rejected by IcebergScanPlanning.rejectServerSideScanPlanning before the procedure runs, although this SINGLE_CALL action only uses snapshots and manageSnapshots().rollbackTo(...).commit(). Route metadata-only procedure targets through getTableHandleForDdl too. This is the separate EXECUTE adapter; the existing DROP/catalog-ALTER thread's callers are already fixed.
There was a problem hiding this comment.
Fixed in 068d3c7. ConnectorExecuteAction now resolves the DDL handle before per-handle procedure dispatch. SINGLE_CALL actions use it directly, so a server-planned REST table is not rejected by the scan-planning read path. DISTRIBUTED rewrites reacquire a scan-capable read handle before file planning. The test uses distinct DDL and scan handles and verifies planRewrite receives the scan handle; all 16 ConnectorExecuteActionTest cases pass.
| this.freshness = ImmutableMap.copyOf(freshness); | ||
| // Build before cache admission, like HivePartitionValues: the cache accounts for the index | ||
| // together with its maps, and readers never observe a partially initialized generation. | ||
| this.sortedRanges = hasComparableKeys(this.items) ? SortedPartitionRanges.build(this.items) : null; |
There was a problem hiding this comment.
[P2] Skip index construction when binary-search pruning is disabled
For a large partitioned Hive/Paimon table with enable_binary_search_filtering_partitions=false and meta.cache.default.partition_view.enable=false, every eager snapshot now calls SortedPartitionRanges.build here, including an unfiltered full scan. That converts every partition key, allocates an index, and sorts all ranges, adding O(P log P) work on every query even though both pruning consumers explicitly refuse to use the index. Previously sorting happened only when filter pruning requested it behind the session switch. Build the index on demand or honor the pruning gate before constructing it.
There was a problem hiding this comment.
Fixed in 068d3c7. The partition-view cache key now includes the binary-search setting. With filtering disabled, the view skips comparability checking and SortedPartitionRanges.build; when enabled later, it uses a separate cached view with the sorted index. This preserves immutable generation and admission-size accounting. Added a switch-and-cache test; all 15 PluginDrivenPartitionViewTest cases pass.
Issue Number: None
Problem Summary: Ordinary Iceberg scans and SQL result-cache validation materialize and copy the full Doris partition view even when native file pruning or a pinned metadata version is sufficient. Cache immutable engine partition views and their pruning index by connector metadata generation, defer conversion for native planning, and obtain SQL cache versions through a source-neutral version provider. Record distinct current, historical and branch/tag references when a statement reads several versions of one table, and revalidate them through the existing snapshot resolver. Reuse statement-retained table handles and combine relation-name discovery through the connector SPI to avoid repeated remote existence and listing requests. Preserve MV/partition metadata consumers, refresh invalidation and frozen-reader schema consistency.
Review repairs: retain heterogeneous display/freshness partition maps without an invalid sorted index, carry entry-specific scalable byte estimates through catalog cache definitions, and align partition-metadata regression settings and the Hive fixture with native planning. Rebase includes upstream source-nullability and the existing Paimon CHAR regression repair; connector API is 20.0. Final rebased validation: 210 scoped unit tests, an overlapping 21-test weighted-cache/runtime pass, and build.sh --fe -j4 with Checkstyle passed. Historical performance and live-SQL validation below predate these review repairs.
Review follow-up: keep Iceberg tableExists authoritative for DDL and use that live preflight before DROP TABLE. Cached/pinned handles remain available to read planning. Added externally dropped-table cases for IF EXISTS and strict DROP, plus cached/pinned read isolation. Keep the partition-rule Env accessor independent of the scheduler import removal so the FE PRs compile together. 96 scoped tests and build.sh --fe with Checkstyle passed.
Review follow-up: use live tableExists for both CREATE TABLE existence probes, including IF NOT EXISTS race recovery. Added stale cached-handle tests for both CREATE modes and failed-create propagation. All 90 DDL routing tests, full FE build and Checkstyle passed.
Iceberg ordinary scans use native file pruning by default. The session variable enable_iceberg_internal_partition_pruning enables Doris internal partition pruning when needed. SQL result caching supports independently versioned Iceberg snapshot and branch/tag references without enumerating partitions.
- Test: Unit Test
- run-fe-ut.sh: 325 targeted metadata, MVCC, partition view, SQL cache and dependent test cases passed
- run-fe-ut.sh: all 85 connector SPI tests passed separately
- build.sh --fe -j4 passed, including Checkstyle and connector architecture gates
- Behavior changed: Yes, defer ordinary Iceberg partition conversion by default and support version-aware external SQL caching
- Does this need documentation: Yes, updated fe/fe-connector/README.md
Measured independently against unoptimized master `e0bf4874070`, with identical Linux AArch64 Release BE, data/footer prewarming, and session settings. Numbers are complete SQL medians, in milliseconds. The baseline column shows two separate baseline runs.
| Scenario | Baseline runs | This PR |
|---|---:|---:|
| 120,000 partitions, warm LIMIT 10 | 389.38–463.29 | 9.69 |
| 120,000 partitions, warm EXPLAIN | 188.19–189.07 | 3.45 |
| 120,000 partitions, warm LIMIT 10 with internal pruning enabled | 333.18–391.80 | 9.76 |
| 1,000 manifests / 10,000 files, cold LIMIT 10 | 925.44–933.77 | 26.50 |
The large-partition protocol enables SQL Cache validation but varies an always-true predicate to avoid result-cache hits. Ordinary LIMIT cases have 40 measured samples; EXPLAIN has 20. Baseline EXPLAIN retention is bounded by using equivalent SELECT warmup, identically for all variants. Function-heavy partition predicates remain approximately 5.5 seconds in this fixture and are not claimed as a benefit.
- Live SQL Cache checks: 170 assertions for current reads, snapshots, timestamps, branch/tag movement, multiple versions of one table, schema evolution/rename, snapshot expiry and ref removal/recreation.
- The final combined tree passed a clean FE build, 997 scoped unit tests without skips, and the Hive streaming / Iceberg scheduled-split / table-handle-reuse regression suites.
Issue Number: None Problem Summary: The Iceberg PR adds public connector SPI after master already advanced the plugin API to 20.0. Rebasing without a further version increment would misrepresent compatibility. Advance the plugin API and surface contract to 21.0. None - Test: Unit Test: ConnectorPluginSurfaceTest, IcebergTableHandleReuseTest, MvccReadSelectorTest, and PluginDrivenPartitionViewTest passed (28 tests). - Behavior changed: No (version declaration reflects the public SPI change). - Does this need documentation: No
### What problem does this PR solve? Issue Number: None Problem Summary: A cold REST Iceberg table with server-side scan planning can pass the live existence check for DROP TABLE, then fail while the query read-handle path rejects local scan planning. Add a DDL handle path that returns an identifier-only Iceberg handle after a live existence check. Keep the query read path unchanged and route DROP and ALTER through the DDL path. ### Release note REST Iceberg DROP TABLE and ALTER TABLE no longer require local scan-planning support. ### Check List (For Author) - Test: Unit Test: IcebergTableHandleReuseTest (10 tests) and PluginDrivenExternalCatalogDdlRoutingTest (90 tests) via run-fe-ut.sh; BUILD SUCCESS. - Behavior changed: Yes, REST Iceberg DDL uses an identifier-only handle after checking table existence. - Does this need documentation: No
### What problem does this PR solve? Issue Number: None Problem Summary: The new metadata-only DDL handle method was absent from the recorded SPI surface and plugin API version. Metadata-only EXECUTE actions still acquired scan-capable read handles, so a cold REST Iceberg table configured for server-side scan planning rejected rollback before execution. Partition views also sorted all partition ranges while binary-search filtering was disabled. Record the DDL method and advance the connector API version, use the DDL handle for single-call procedures while retaining a scan handle for distributed rewrites, and key partition views by the index setting so disabled queries skip index construction without poisoning enabled queries. ### Release note Metadata-only Iceberg EXECUTE actions support server-planned REST tables. External partition views skip sorted-index construction when binary-search partition filtering is disabled. ### Check List (For Author) - Test: Unit Test: ConnectorMetadataSurfaceTest (4), ConnectorPluginSurfaceTest (2), IcebergTableHandleReuseTest (10), PluginDrivenPartitionViewTest (15), and ConnectorExecuteActionTest (16) via run-fe-ut.sh; strengthened ConnectorExecuteActionTest (16) rerun. Full FE build via build.sh --fe -j4, including Checkstyle, passed after temporarily preserving existing output plugin configs. - Behavior changed: Yes, metadata-only EXECUTE uses the DDL handle path and disabled binary-search partition filtering does not build the sorted index. - Does this need documentation: No
3bde61a to
068d3c7
Compare
|
run buildall |
|
/review |
There was a problem hiding this comment.
Found one new P2 correctness regression: independent raw-table eviction can make a warm latest query read an older snapshot than the preceding cold query. The inline finding describes a concrete sequential trigger. All eight existing P2 concerns were revalidated as fixed at this head; none is reposted, and no existing P0/P1 blocker remains in the supplied threads. There was no additional user-provided focus.
Static flow reviewed: binding -> selector-specific snapshot/schema -> deferred or eager partition view -> pruning/scan; SQL-cache dependency capture -> MD5 identity -> original-selector revalidation. Two normal reviewers covered all 70 changed paths, with a separate reviewer auditing generation and deferral boundaries. The complete proposed payload was frozen before the final independent pass.
Critical checkpoint conclusions:
- Goal and proof: the change versions reusable partition projections and SQL-cache metadata, and defers ordinary Iceberg partition enumeration. Targeted unit/regression coverage supports those paths; the inline cache-generation regression remains.
- Scope and reuse: changes stay within FE connector metadata, cache/planning consumers and related tests; existing cache ownership, invalidation and sorted-range machinery are reused.
- Concurrency and locks: request binding and asynchronous scan pumps share concurrent statement memos and retained leases; the deferred supplier memoizes its first successful load. No additional lock-order defect was established. The finding needs no concurrency.
- Lifecycle: planning-only values are released at endPlanning; table metadata/leases survive until statement completion after pump shutdown. Cold/warm generation selection is inconsistent as reported. No static-initialization issue applies.
- Configuration: the new internal-pruning session switch defaults false and is captured per statement; changing it affects subsequent statements in the current SQL session. Binary-search state is part of the partition-view key.
- Compatibility: optional SPI defaults preserve fallback behavior; plugin API 22.0 and both reflected inventories are updated. No persisted format change is introduced.
- Thrift/protobuf: no definitions changed; the cross-release field-ID audit is inapplicable.
- Parallel paths: checked native/eager, LIST/RANGE, current/historical/branch/tag, empty/system tables, Hive/Paimon/Hudi fallback, DDL/query handles and SINGLE_CALL/DISTRIBUTED procedures. Gateways retain conservative eager defaults.
- Conditions and errors: native unknown selections remain scan-all rather than empty; partition rules force exact metadata or an explicit retry. Missing-table handling remains distinct from load failures; unreliable SQL-cache versions disable caching.
- Test coverage: inspected selector immutability, identity/schema/rollback, refresh and recreation, weighted admission, invalidation during load, deferred concurrency/failure, block rules and DDL routing tests. The multi-statement eviction sequence is missing; physical SQL-result-cache/ref-expiry integration is chiefly covered through mocks.
- Results: the added ordered regression output agrees with the inserts, branch/tag movement and recreation. Output-generation provenance was not independently verified.
- Observability: EXPLAIN labels native partition counts unknown; existing cache statistics/admission diagnostics, scan profiles and partition-conversion warnings remain available.
- Transactions and persistence: existing DDL/EditLog/invalidation boundaries remain; no new persisted transaction state or replay protocol is added.
- Writes and crash behavior: connector-owned mutation and distributed rewrite commit protocols remain in place; no new write atomicity or crash-recovery defect was established statically.
- FE/BE variables: no new transmitted field or send path is introduced; SQL versions enter the FE cache key while existing PCache scalars are reused.
- Performance: native scans avoid FE enumeration, eligible views share conversion/index work, partition recording avoids full-set hashing, and MV unions are delayed until needed. Weighted sizing and the disabled-index gate address earlier reported regressions.
- Further sweep: checked failed cache-validation continuation, prepared selector resets, cache membership/admission, shared-item mutation, late partition rules and worker ownership. No second verified finding remains.
Limits: static review only, as required. No build, unit/regression test, cluster run or performance benchmark was executed. Author-reported test passes are not claimed as independently verified. Convergence means no further valuable finding in the completed review scope; it does not mean the reported P2 has been fixed.
Review converged after 3 round(s).
| // Handle/schema resolution may have frozen an older generation before another query's REFRESH. | ||
| // A newer shared pin must not point outside that frozen metadata, nor may this reader publish its | ||
| // old pin over the refreshed cache entry. | ||
| pin = latestSnapshotPin(loadTable(session, iceHandle)); |
There was a problem hiding this comment.
[P2] Keep warm snapshot pins aligned after raw-table eviction
With SQL caching disabled, a long table-cache TTL and meta.cache.iceberg.table.capacity=1, read A cold and then warm at S1, read cold B until its raw entry evicts A, append to A externally to create S2, and read A twice. Cold B and the first subsequent A read now freeze a table and take this branch without populating/replacing the separate latest-snapshot cache, so its A=S1 entry survives. The first A read therefore returns S2, but the next warm handle lookup returns only an identifier and the else branch reuses S1; buildScan then applies that old snapshot and previously visible rows disappear. Before this change, cold B also populated the latest cache and displaced A's old pin. Resolve warm pins from the same retained raw-table generation, or coordinate these cache generations while preserving old in-flight readers. Please cover this cold/warm eviction sequence in the reuse tests.
There was a problem hiding this comment.
Fixed in 4cedfb0. The latest snapshot pin now comes from the same raw table cache generation whenever that cache exists; the independent latest-snapshot cache remains for catalogs without a raw table cache. Added a capacity-1 A/B eviction regression with an external append: the new test failed on the previous code (warm A returned S1 after cold A saw S2) and all 11 IcebergTableHandleReuseTest cases pass after the fix. Existing in-flight readers still use their frozen statement table.
|
run buildall |
…tions ### What problem does this PR solve? Issue Number: None Problem Summary: A cold read of an Iceberg table retained a raw table without filling the separate latest-snapshot cache. After capacity eviction and an external append, a cold read could use the new table generation while the next warm read reused the old snapshot pin, hiding newly visible rows. For catalogs with a raw table cache, derive the pin from that same cached table generation. Keep the independent latest-snapshot cache only for catalogs without a raw table cache. Add a regression covering cold and warm reads around capacity eviction and an external append. ### Release note Fix stale Iceberg snapshot selection after table-cache eviction. ### Check List (For Author) - Test: Unit Test: IcebergTableHandleReuseTest (11) via run-fe-ut.sh; confirmed the new case fails on the previous implementation and passes after the fix. FE Maven package and Checkstyle passed via build.sh --fe; existing output plugin config caused the packaging step to exit 1. - Behavior changed: Yes, a warm read pins the snapshot from its retained raw table generation. - Does this need documentation: No
|
run buildall |
|
/review |
There was a problem hiding this comment.
Static review of PR #69299 at 4cedfb0c5b8518c5ab2412fd3b7fb3b5ea5e57fb: no new substantiated findings. All 70 authoritative changed paths and their relevant callers/consumers were covered by the main reviewer, two complete-scope reviewers and an additional risk reviewer. Review converged after 2 rounds.
The nine existing P2 findings were independently rechecked as addressed: heterogeneous display-key indexes, scalable view accounting, live DROP/CREATE existence checks, metadata-only DROP/ALTER/EXECUTE handles, SPI baseline/version updates, disabled binary-search index construction, and raw-table/latest-pin eviction alignment. No existing P0/P1 finding remains applicable, and no duplicate inline comments are added. No additional user focus was supplied.
Critical checkpoint conclusions:
- Goal and evidence: Ordinary Iceberg planning can defer engine partition conversion, and SQL-cache validation uses pinned scalar metadata rather than partition enumeration. Focused unit and regression fixtures cover generation retention and read/ref/refresh/recreation behavior; this is a static conclusion.
- Scope and reuse: The changes form a coherent connector SPI, metadata-view and result-cache change. Existing statement scopes, cache infrastructure and snapshot resolvers are reused; the DDL adapters preserve separate live existence semantics.
- Concurrency and locks: Query/scan metadata reuse and concurrent refresh/eviction retain generation-owned leases. Deferred initialization is memoized, and map/index publication is atomic at the object level. No new thread, heavy critical section or lock-order defect was identified.
- Lifecycle and initialization: In-flight readers retain their frozen tables after cache invalidation; statement cleanup releases ownership. No reachable deferred-loader use after statement cleanup was found. C++ static initialization is unaffected.
- Configuration:
enable_iceberg_internal_partition_pruningdefaults to false and is captured through existing session-property export. Subsequent statements observe changes. The converted-view key includes the binary-search setting and timezone. - Compatibility: Additive SPI defaults preserve other connectors' fallback paths. Both recorded public-surface baselines are updated, and the plugin API/test assertion advance to 22.0. No storage-format change was found.
- Thrift/protobuf IDs: Not applicable; no definitions change.
- Parallel paths: Latest, historical, branch/tag, scoped/unscoped and enabled/disabled-cache reads were checked alongside LIST/RANGE views, eager/native pruning, synchronous/streamed scans and CREATE/DROP/ALTER/EXECUTE routing. No additional inconsistent path was substantiated.
- Special conditions and errors: Native selection remains distinct from an empty scan. MV compensation receives ALL_PARTITIONS for unknown selections. Existing partition rules require eager metadata; newly installed rules reject unknown counts. Missing/expired references and version failures invalidate or disable result caching rather than reuse an unsafe version.
- Test coverage: Added tests cover eviction, schema/ref movement, original-selector copies, cache invalidation, deferred failure/concurrency, weighted admission, native-count semantics and DDL routing. The new integration suite disables SQL caching; reference-aware SQL-cache coverage is primarily unit tests plus author-reported manual validation.
- Expected results: The added regression output agrees with the suite's explicit inserted values, branch movement and table recreation. Actual output-generation provenance and passing test outcomes were not independently established.
- Observability: EXPLAIN explicitly reports unknown native partition counts. Existing cache statistics and table-load error/log context remain available; no additional observability defect was identified.
- Transactions and persistence: No EditLog payload, replay format or commit protocol changes. DDL cleanup/logging/invalidation remain on their existing paths; converted views are ephemeral.
- Writes and crash behavior: No data-write or transaction algorithm changes. Distributed rewrites reacquire a scan-capable handle; metadata-only procedures use the DDL handle. No new atomicity or resource-release defect was substantiated.
- FE/BE propagation: No new BE protocol field or transmitted variable is required; the new switch is consumed through FE connector session properties.
- Performance: The ordinary native paths avoid partition enumeration, version tokens are constant-size, converted maps/indexes are reused, and candidate-free MV checks avoid partition unions. The sorted-index gate and custom admission estimator address the prior performance findings. Reported benchmark figures were not rerun or independently confirmed.
- Other concerns and limits: The final changed-file, candidate and risk sweep found no unresolved concrete candidate or uncovered assigned scope. All conclusions are limited to static inspection: no builds, tests or runtime experiments were performed, as required by this review contract.
Review converged after 2 round(s).
FE UT Coverage ReportIncrement line coverage |
What problem does this PR solve?
Issue Number: None
Problem Summary: Ordinary Iceberg scans and SQL result-cache validation materialize and copy the full Doris partition view even when native file pruning or a pinned metadata version is sufficient. Cache immutable engine partition views and their pruning index by connector metadata generation, defer conversion for native planning, and obtain SQL cache versions through a source-neutral version provider. Record distinct current, historical and branch/tag references when a statement reads several versions of one table, and revalidate them through the existing snapshot resolver. Reuse statement-retained table handles and combine relation-name discovery through the connector SPI to avoid repeated remote existence and listing requests. Preserve MV/partition metadata consumers, refresh invalidation and frozen-reader schema consistency.
Main benefit
Remove partition-count-proportional work from ordinary Iceberg planning and SQL Cache validation. On the independently measured 120,000-partition fixture, warm LIMIT 10 falls from 389–463 ms to 9.69 ms, and warm EXPLAIN from 188–189 ms to 3.45 ms. On the 1,000-Manifest fixture, cold LIMIT falls from 925–934 ms to 26.50 ms. The independent A/B table and cache protocol are below; both variants use the same Release BE.
Ordinary scans use native Iceberg pruning without first converting every partition into a Doris representation. Consumers that need an internal partition view reuse an immutable, generation-specific view and index. SQL Cache validates the actual selected table/snapshot/schema version without enumerating partitions, including independently tracked historical, branch and tag references. DDL existence probes remain live, so cached query handles cannot make CREATE/DROP decisions stale.
The 120,000-partition experiment deliberately avoids SQL result-cache hits, so the improvement measures planning and metadata validation rather than returning cached query results. Function-heavy partition predicates that still require value evaluation are not claimed to improve.
Release note
Iceberg ordinary scans use native file pruning by default. The session variable enable_iceberg_internal_partition_pruning enables Doris internal partition pruning when needed. SQL result caching supports independently versioned Iceberg snapshot and branch/tag references without enumerating partitions.
Check List (For Author)
Latest DDL review-repair validation at
b44b384627e: all 90 DDL-routing tests and a fullbuild.sh --fe -j4with Checkstyle passed. Tests cover CREATE/CREATE IF NOT EXISTS after an external drop and reject a stale cached handle as proof of a concurrent creator. Earlier metadata/SPI/live SQL checks listed below cover their respective implementation revisions; the performance table predates the subsequent rebase and review repairs and is not a fresh A/B of this head. CI status remains tracked by the checks on this PR.Performance validation
Measured independently against unoptimized master
e0bf4874070, with identical Linux AArch64 Release BE, data/footer prewarming, and session settings. Numbers are complete SQL medians, in milliseconds. The baseline column shows two separate baseline runs.The large-partition protocol enables SQL Cache validation but varies an always-true predicate to avoid result-cache hits. Ordinary LIMIT cases have 40 measured samples; EXPLAIN has 20. Baseline EXPLAIN retention is bounded by using equivalent SELECT warmup, identically for all variants. Function-heavy partition predicates remain approximately 5.5 seconds in this fixture and are not claimed as a benefit.
Additional validation