924060929 commented on PR #66529: URL: https://github.com/apache/doris/pull/66529#issuecomment-5212619161
Verified the latest revision (`5662a6f5`). All three points from my previous comment are addressed — thanks: 1. **P1 (delete-manifest enumeration per query) — resolved.** `cachedApplicableEqualityDeleteFieldIds` now routes the equality-delete field-id projection through a snapshot-scoped cache (`IcebergManifestCache.equalityDeleteFieldIds`, keyed by `(tableLocation, snapshotId)`), and the compact projection is used **even when the full manifest-cache feature gate is off** — which is exactly the default configuration, so the per-query re-read of every delete manifest is gone. The design is sound: snapshot contents are immutable so the key is safe; loader failures deliberately escape and are not memoized (retry after transient storage errors, covered by `equalityDeleteFieldIdFailureIsNotCached`); `invalidateAll` on REFRESH CATALOG clears it; capacity is bounded by the shared cache spec. 2. **`resolveSourceFieldPath` `uniqueId < 0` silent top-level fallback — resolved.** It now hard-fails (returns null → non-partitioned fallback) instead of assuming top-level, and `reconstructUnstampedNestedSourceHardFails` pins it. The only other generic-path consumer is the test seam (`tableColumns == null`), which keeps its exact prior lookup — safe. 3. **MERGE/UPDATE nested sources — resolved.** `reconstructPartitionFields` now resolves the expr id by the top-level column's root id when `columnIdToExprId` is populated, with the nested child id selecting the path inside it; `reconstructNestedSourceUsesTopLevelIdMapFromProductionPath` covers the production map shape. No remaining P1 from the FE side. Two optional notes, both non-blocking: - The requiredness fence (`selectedHistoryRequiresMissingRequiredFieldRejection`) intentionally still gates on any historical schema without proving snapshot ancestry — fine as a conservative design, just noting it remains the widest rolling-upgrade availability surface in this PR. - The early-exit idea (stop reading delete manifests once all seen field ids are present in `scanSchema`) is now mostly moot given the per-snapshot cache, but it would still help the first query on a delete-heavy table after cache eviction/catalog refresh. -- 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]
