enhance: classify iceberg planning errors - #608
Conversation
…ions
The lance and iceberg cxx bridges reported every failure by throwing a
string-only exception (LanceException/IcebergException) out of the
library, which (a) violated the no-exceptions-across-the-boundary
contract -- four LanceTableReader read methods and api::Reader had no
catch at all, so bridge errors escaped into consumers as foreign
exceptions and collapsed to a generic internal error -- and (b) erased
the error class: a corrupt lance file, a missing dataset, and an S3
throttle all surfaced as one opaque IOError, so permanent failures were
indistinguishable from retriable ones.
Rust side (producer owns classification):
- new bridge_error.rs: BridgeError embeds a classification code into the
error message with the same marker/parser the vortex bridge already
uses; classify_lance_error maps lance::Error variants -- the NotFound
family to file-not-found, CorruptFile/Schema{,Mismatch} to a
data-corrupt code, NotSupported to not-supported, and the
lance-declared-retryable RetryableCommitConflict/TooMuchWriteContention
to the transient-throttling tag; the IO variant downcasts its
object_store source (NotFound / PermissionDenied+Unauthenticated /
Precondition / NotSupported). Anything not positively identified stays
untagged and lands in the conservative non-retriable bucket; no
retriability is invented. InvalidInput is deliberately NOT tagged as
caller input pending a producer-site audit.
- BatchFutStreamReader::next() wraps stream errors in BridgeError so the
classification survives arrow FFI stringification -- this is the only
choke point mid-scan read errors (the hot transient case) pass through.
C++ side:
- shared bridge_error.{h,cpp}: decodes the marker back into a structured
arrow::Status (file-not-found -> IOError+ENOENT detail -> ObjectNotExist;
extend codes -> ExtendStatusDetail; bridge-private data-corrupt ->
Status::Invalid; not-supported -> Status::NotImplemented; no marker ->
plain IOError) plus a translating RecordBatchReader wrapper for live
streams. The vortex bridge delegates to it; vortex public API and
behavior are unchanged.
- lance_bridge/iceberg_bridge: all fallible APIs now return
arrow::Result/arrow::Status; the 23 throw sites and both exception
types are gone. Estimate/IOStats keep their best-effort degrade
semantics.
- consumers (lance_table_reader/writer, lance_format, iceberg_format,
loon tool, tests, benchmarks) converted to status propagation;
read_with_range wraps its live stream in the translating reader, and
the drained-stream paths (get_chunk/get_chunks/take) decode stream
errors on failure.
- behavior fix in LanceTableWriter::Close: the create-new-dataset
fallback now triggers only on a classified not-found; previously any
open failure (auth error, corruption, transient IO) was treated as
"dataset does not exist" and silently created a fresh dataset.
Tests: new lance_bridge_error_test pins the decoder table (not-found ->
2017 ObjectNotExist, transient tag -> 2045 retryable, corrupt -> 2024,
untagged/unknown -> 2044, marker never leaks into messages) and an
end-to-end open of a nonexistent dataset classifying as not-found.
Existing vortex error tests unchanged and passing; lance/iceberg suites
pass (78 ran / 69 passed / 9 skipped for missing cloud credentials).
issue: milvus-io#595, milvus-io/milvus#50903
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: zhenshan.cao <zhenshan.cao@zilliz.com>
Adopts the adversarial-review findings on milvus-io#597: - High (object_store downcast coupling + untagged 429/503): the typed carrier of the post-retry HTTP status (client::retry::RetryError) is pub(crate) in object_store and cannot be downcast, so the status is recovered from RequestError::Status's stable Display pattern ("non-2xx status code: NNN"): 408 -> transient-timeout, 429 -> throttling, 500/502/503/504 -> service. Fail-safe by construction: a reworded message degrades to untagged/non-retriable, never the reverse; unknown 4xx stay untagged (test-pinned). The version coupling is now a compile-time pin: a unit test constructs LanceError::from(object_store::Error), which stops compiling if the bridge's object_store ever diverges from lance's. - Medium (writer stream leak): LanceTableWriter::Close now guards the exported stream with RAII; the Rust write entry points take ownership immediately (ptr::replace with an empty stream, making the guard a no-op on those paths), so the guard only fires on the error returns before the stream reaches Rust -- exactly the paths this PR added. - Medium (FieldNotFound classified as ENOENT): FieldNotFound no longer maps to file-not-found -- ENOENT drives create-if-missing in the writer, so a projection typo could have triggered dataset creation. It stays untagged (conservative), with a rust test pinning that. - Medium (SchemaMismatch != corruption): Schema/SchemaMismatch moved out of the data-corrupt bucket to untagged; producer sites are mixed (library-assembled schemas vs user projections), so no input-blame either. CorruptFile alone remains data-corrupt. - Medium (missing writer tests): two tests anchor both directions of the Close decision: a classified not-found creates the dataset; an EACCES open failure propagates and creates nothing. - Low (TranslateBridgeStatus downgraded non-IOError statuses): bridge errors only travel as IOError strings, so non-IOError statuses (Invalid / OutOfMemory / NotImplemented from arrow itself) now pass through with their StatusCode intact instead of being rewritten to IOError -- an OutOfMemory would have become non-retriable. Test-pinned. Verified: cargo test (release) 4/4 classification tests; full make build clean; lance/bridge suites 66 ran / 56 passed / 10 skipped (cloud credentials); the three new tests pass in isolation. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: zhenshan.cao <zhenshan.cao@zilliz.com>
…l-path HTTP failures - High (mid-scan decode regression): the round-1 fix guarded TranslateBridgeStatus with !IsIOError(), but arrow-rs's C-stream exporter maps Rust stream errors to EINVAL, so mid-scan bridge errors arrive as Status::Invalid still carrying the marker -- the guard passed them through undecoded, leaking the marker and collapsing transients into DataFormatBroken. Discrimination is now on MARKER PRESENCE: any status whose message carries the marker is decoded regardless of StatusCode; marker-less statuses pass through untouched (the original no-downgrade property, still test-pinned). New test drives Invalid(marker+109) -> retryable transient detail and Invalid(marker+12) -> ENOENT. - Medium x2 (credential paths stringify the typed status): the GCP impersonation token requests and the Aliyun STS/OIDC fetches now prefix the canonical "non-2xx status code: NNN:" pattern while the typed StatusCode is still in hand, so the downstream classifier recovers the class; 401/403 now map to access-denied(105) alongside the transient codes (test-pinned via the canonical pattern). Verified: cargo test --release 4/4; full build clean; bridge error suites 9/9; lance suites 27 passed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: zhenshan.cao <zhenshan.cao@zilliz.com>
The marker is no longer vortex-only (this PR added the lance-native producer), so the name now reflects its role as the shared rust-bridge error channel. All three producers/consumers renamed in lockstep (filesystem_c.rs, bridge_error.rs, bridge_error.cpp) plus tests; the marker never persists nor crosses process boundaries, so the rename has no compatibility impact. Verified: cargo test 4/4; bridge/lance/vortex error suites 13/13. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: zhenshan.cao <zhenshan.cao@zilliz.com>
Signed-off-by: xiaofanluan <xiaofan.luan@zilliz.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: xiaofanluan <xiaofan.luan@zilliz.com>
The new "Test Rust bridge error classifier" step fails with lance-encoding build-script-build (exit status: 1) Error: Could not find `protoc`. `cargo test` runs under the dev profile, so it does not reuse the artifacts corrosion produced during the Release build and instead rebuilds the dependency tree, which re-runs lance-encoding's build script (prost-build needs protoc). The Build step gets protoc from conan -- the same job logs `Conan: Component target declared 'protobuf::libprotoc'` -- but the standalone cargo step does not inherit that environment. Installing protobuf-compiler alongside libaio-dev is the smallest fix that does not depend on the conan layout. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011HDgMok3nR8ZNugKWWQQK2 Signed-off-by: xiaofanluan <xiaofan.luan@zilliz.com>
classify_lance_error handled LanceError::IO but let Wrapped and External
fall through to `_ => None`, which discards a classification that is
already present one box deeper.
This is not a coarseness gap, it is a live loss on the most common
retriable path. lance-io's batch read scheduler stashes the failing
task's error and re-wraps it when the batch drops:
// lance-io/src/scheduler.rs
Err(err) => { self.err.get_or_insert(Box::new(err)); }
...
impl Drop for MutableBatch { ... Err(Error::wrapped(self.err.take().unwrap())) }
and `impl From<object_store::Error> for lance::Error` produces
`IO { source }`. So on any batched read an S3 throttle arrives as
Wrapped(IO(object_store::Generic)) rather than IO(..), and was reported
as a permanent StorageError instead of a retriable one. The encoding
decoder wraps the same way (lance-encoding/src/decoder.rs).
Both wrappers now downcast their box: object_store::Error first, then
LanceError recursively (each step strips one layer, so it terminates).
The object_store arm is extracted into classify_object_store_error so
the two paths share it.
Cloned { message: String } stays unclassifiable by construction -- lance
stringifies errors when cloning them across task boundaries, so the type
is gone before the bridge ever sees it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011HDgMok3nR8ZNugKWWQQK2
Signed-off-by: xiaofanluan <xiaofan.luan@zilliz.com>
iceberg was the last unclassified producer reachable from the Rust
bridges: iceberg_bridge.cpp already decoded the marker, but nothing on
the Rust side ever emitted one, so every failure crossed as an untagged
string and landed on StorageError/2044.
Scope is narrower than it looks. iceberg's Rust does planning only
(iceberg_plan_files); the data files it returns are read by the C++
parquet reader on the C++ ArrowFileSystem, which already classifies. So
this covers metadata/manifest access and snapshot resolution, not reads.
The classification has to come from the source chain, not from iceberg's
own kind. iceberg-storage-opendal collapses every IO failure into
Error::new(ErrorKind::Unexpected, "Failure in doing io operation")
.with_source(opendal_error)
so the iceberg kind carries no IO signal at all -- but the typed
opendal::Error survives as a source. classify_anyhow_error walks the
anyhow chain (anyhow keeps concrete types through `?`) and downcasts to
iceberg::Error then opendal::Error, first positive identification wins.
opendal is the better of the two IO backends to classify against: it has
a typed ErrorKind *and* its own is_temporary() bit, so unlike the
object_store path nothing has to be recovered from prose.
Entry point converts once at the boundary: iceberg_plan_files is now a
thin wrapper that calls the unchanged anyhow-based body and maps the
error through BridgeError::from, so `?` stays ergonomic inside.
Deliberately left untagged: ConfigInvalid and iceberg's write-path
conflicts. The first is a caller/operator mistake with no user-error code
on this channel yet; the second would dilute the CAS-specific conflict
code and plan_files is read-only anyway.
Tests: iceberg and opendal kind tables, the anyhow chain walk, and a
version pin that reproduces from_opendal_error's exact shape -- an
opendal version skew between this crate and iceberg-storage-opendal
would make the downcast silently return None, and that test catches it.
A compile-time pin is not possible: with_source accepts any error type.
Depends on milvus-io#597 (introduces bridge_error.rs); branched from it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011HDgMok3nR8ZNugKWWQQK2
Signed-off-by: xiaofanluan <xiaofan.luan@zilliz.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: xiaofan-luan The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #608 +/- ##
==========================================
+ Coverage 75.95% 76.03% +0.07%
==========================================
Files 168 168
Lines 16672 16697 +25
Branches 2509 2519 +10
==========================================
+ Hits 12664 12695 +31
+ Misses 4008 4002 -6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Problem
iceberg was the last unclassified producer reachable from the Rust bridges.
iceberg_bridge.cppalready decodes the marker, but nothing on the Rust side ever emitted one, so every planning failure crossed as an untagged string and landed onStorageError/2044 — a throttle on manifest reads looked exactly like a malformed table.Scope is narrower than it sounds. iceberg's Rust does planning only (
iceberg_plan_files); the data files it returns are read by the C++ parquet reader on the C++ArrowFileSystem, which already classifies. So this covers metadata/manifest access and snapshot resolution, not the read path.Why the classification has to come from the source chain
iceberg-storage-opendalcollapses every IO failure into a single kind:So
iceberg::ErrorKindcarries no IO signal at all — everything isUnexpected. What survives is the typedopendal::Errorin the source chain.classify_anyhow_errorwalks the anyhow chain (anyhow keeps concrete types through?) and downcastsiceberg::Errorthenopendal::Error; first positive identification wins.opendal is the friendlier of the two IO backends to classify against — it has a typed
ErrorKindand its ownis_temporary()bit, so unlike theobject_storepath nothing has to be recovered from prose.opendal::NotFound,iceberg::TableNotFound/NamespaceNotFoundObjectNotExistopendal::PermissionDeniedopendal::ConditionNotMatch,iceberg::PreconditionFailedopendal::RateLimitedis_temporary()opendal::Unsupported,iceberg::FeatureUnsupportedNotImplementediceberg::DataInvalidDataFormatBrokenDeliberately left untagged
opendal::ConfigInvalid— a caller/operator mistake, but there is no user-error code on this channel yet. Mislabelling it as a storage failure is worse than leaving it in the conservative bucket; revisit when the user/system axis lands (enhance: define storage error taxonomy and preserve diagnostics #603).TableAlreadyExists,NamespaceAlreadyExists,CatalogCommitConflicts) — reusing the CAS-specific conflict code would dilute it, andplan_filesis read-only anyway.Entry point
iceberg_plan_filesbecomes a thin wrapper that calls the unchanged anyhow-based body and maps once throughBridgeError::from, so?stays ergonomic inside and classification happens at exactly one place.Verification
cargo test --lib bridge_error::tests→ 12 passed (4 new).make buildclean.from_opendal_error's exact shape and asserts the chain walk recovers the code. An opendal version skew between this crate andiceberg-storage-opendalwould make the downcast silently returnNoneand degrade every iceberg IO failure to untagged — this catches that. A compile-time pin is impossible here becausewith_sourceaccepts any error type.Not covered
The iceberg read path needs nothing here (it is C++), and
ConfigInvalidwaits on the user/system axis. paimon remains the last unclassified stack (its own opendal, tracked with #602).🤖 Generated with Claude Code
https://claude.ai/code/session_011HDgMok3nR8ZNugKWWQQK2