rust/lance-table/src/io/commit/external_manifest.rs (2)

404-409: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Log the stale-finalized-path fallback.

The NotFound branch silently falls back to default_resolve_version with no runtime log, even though this indicates the external store's finalized-path record is stale/wrong — an unexpected condition. The rest of this file (e.g. the put_if_exists failure path) already uses warn! for comparable best-effort/unexpected situations, so a similar log here would help operators detect a systemically stale external store instead of it silently degrading every lookup.

As per coding guidelines, "Choose log levels by audience: use debug! for routine/high-frequency operations, info! for infrequent operator-visible state changes, and warn! for unexpected conditions" and "Log warnings for silent no-op skipped operations, but omit warnings before errors because the error message is sufficient."

📝 Suggested log addition

             Err(ObjectStoreError::NotFound { .. }) => {
                 // The external store may hold a stale finalized V2 path while
                 // the object store still has the manifest at the V1 location.
+                warn!(
+                    "finalized manifest path {} for version {} not found in object store; falling back to default resolution",
+                    location.path, location.version
+                );
                 default_resolve_version(base_path, location.version, object_store).await
             }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@rust/lance-table/src/io/commit/external_manifest.rs` around lines 404 - 409,
Add a warn!-level log in the NotFound branch before calling
default_resolve_version, indicating that the external store’s finalized path is
stale and the V1/default resolution fallback is being used. Keep the existing
fallback and other error handling unchanged, and include relevant path/version
context in the warning.

Source: Coding guidelines

---

507-525: 🚀 Performance & Scalability | 🔵 Trivial

Verified finalized-path flow looks correct.

Matches the tested behavior (happy path fills in missing size/e_tag, V1 fallback on NotFound). Note this adds a mandatory HEAD round-trip to every "resolve latest" call that lands on a finalized manifest (confirmed by the s3_test.rs read_iops bump from 1 to 2) — an accepted correctness-over-latency trade-off, but worth keeping in mind for high-frequency open/checkout paths under load.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@rust/lance-table/src/io/commit/external_manifest.rs` around lines 507 - 525,
Preserve the finalized-manifest handling in verify_finalized_manifest_location,
including the HEAD-based metadata resolution and V1 NotFound fallback. Do not
remove or bypass this verification when resolving the latest manifest, and
retain the existing behavior that fills missing size and e_tag values.