From a9823644c559616854ef2693cf96be386fa8d53f Mon Sep 17 00:00:00 2001 From: "zhenshan.cao" Date: Mon, 27 Jul 2026 14:28:15 -0700 Subject: [PATCH 1/9] enhance: classify lance/iceberg bridge errors and stop leaking exceptions 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/milvus-storage#595, milvus-io/milvus#50903 Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: zhenshan.cao --- cpp/benchmark/benchmark_format_read.cpp | 16 +- cpp/benchmark/benchmark_storage_layer.cpp | 22 +- cpp/src/format/bridge/rust/.gitignore | 3 + .../format/bridge/rust/include/bridge_error.h | 64 +++++ .../bridge/rust/include/iceberg_bridge.h | 33 +-- .../format/bridge/rust/include/lance_bridge.h | 73 +++--- .../format/bridge/rust/src/bridge_error.cpp | 153 ++++++++++++ .../format/bridge/rust/src/bridge_error.rs | 149 ++++++++++++ .../format/bridge/rust/src/iceberg_bridge.cpp | 24 +- .../format/bridge/rust/src/lance_bridge.cpp | 218 ++++++++---------- .../bridge/rust/src/lance_bridgeimpl.rs | 207 +++++++++-------- cpp/src/format/bridge/rust/src/lib.rs | 7 +- .../format/bridge/rust/src/vortex_bridge.cpp | 135 +---------- cpp/src/format/iceberg/iceberg_format.cpp | 2 +- cpp/src/format/lance/lance_format.cpp | 6 +- cpp/src/format/lance/lance_table_reader.cpp | 110 +++++---- cpp/src/format/lance/lance_table_writer.cpp | 29 ++- cpp/test/format/external_table_arn_test.cpp | 18 +- cpp/test/format/external_table_test.cpp | 26 ++- cpp/test/format/format_reader_cache_test.cpp | 5 +- .../format/iceberg/iceberg_bridge_test.cpp | 26 +-- .../iceberg/iceberg_integration_test.cpp | 20 +- .../format/lance/lance_bridge_error_test.cpp | 112 +++++++++ cpp/test/format/lance/lance_table_test.cpp | 20 +- cpp/test/tools/loon_test.cpp | 16 +- cpp/tools/loon.cpp | 29 +-- 26 files changed, 965 insertions(+), 558 deletions(-) create mode 100644 cpp/src/format/bridge/rust/include/bridge_error.h create mode 100644 cpp/src/format/bridge/rust/src/bridge_error.cpp create mode 100644 cpp/src/format/bridge/rust/src/bridge_error.rs create mode 100644 cpp/test/format/lance/lance_bridge_error_test.cpp diff --git a/cpp/benchmark/benchmark_format_read.cpp b/cpp/benchmark/benchmark_format_read.cpp index 0e22b4d0a..32012d0bf 100644 --- a/cpp/benchmark/benchmark_format_read.cpp +++ b/cpp/benchmark/benchmark_format_read.cpp @@ -335,16 +335,12 @@ class FormatReadBenchmark : public FormatBenchFixtureBase<> { arrow::Result PrepareIcebergReaderFile() const { ARROW_ASSIGN_OR_RAISE(auto table_uri, MakeIcebergTableUri(GetUniquePath("iceberg_read_test"))); - iceberg::IcebergTestTableInfo table_info; - std::vector file_infos; - try { - auto storage_options = iceberg::ToStorageOptions(fs_config_); - table_info = - iceberg::CreateTestTable(table_uri, static_cast(GetLoaderNumRows()), false, {}, storage_options); - file_infos = iceberg::PlanFiles(table_info.metadata_location, table_info.snapshot_id, storage_options); - } catch (const std::exception& e) { - return arrow::Status::IOError("Failed to create Iceberg benchmark table: ", e.what()); - } + auto storage_options = iceberg::ToStorageOptions(fs_config_); + ARROW_ASSIGN_OR_RAISE( + auto table_info, + iceberg::CreateTestTable(table_uri, static_cast(GetLoaderNumRows()), false, {}, storage_options)); + ARROW_ASSIGN_OR_RAISE(auto file_infos, + iceberg::PlanFiles(table_info.metadata_location, table_info.snapshot_id, storage_options)); if (file_infos.empty()) { return arrow::Status::Invalid("Iceberg PlanFiles returned no files"); diff --git a/cpp/benchmark/benchmark_storage_layer.cpp b/cpp/benchmark/benchmark_storage_layer.cpp index 0921f9e1f..47a55f88a 100644 --- a/cpp/benchmark/benchmark_storage_layer.cpp +++ b/cpp/benchmark/benchmark_storage_layer.cpp @@ -310,11 +310,7 @@ class StorageLayerFixture : public FormatBenchFixtureBase<> { ArrowArrayStream stream; ARROW_RETURN_NOT_OK(arrow::ExportRecordBatchReader(batch_reader, &stream)); - try { - auto dataset = lance::BlockingDataset::WriteDataset(lance_uri, &stream, storage_options); - } catch (const lance::LanceException& e) { - return arrow::Status::IOError("Lance write failed: ", e.what()); - } + ARROW_RETURN_NOT_OK(lance::BlockingDataset::WriteDataset(lance_uri, &stream, storage_options).status()); return arrow::Status::OK(); } @@ -539,13 +535,13 @@ BENCHMARK_DEFINE_F(StorageLayerFixture, LanceNative_OpenRead)(::benchmark::State // Lambda to read lance dataset auto read_lance = [&](bool collect_stats, int64_t& out_rows, int64_t& out_bytes) -> arrow::Status { - auto dataset = lance::BlockingDataset::Open(lance_uri, storage_options); + ARROW_ASSIGN_OR_RAISE(auto dataset, lance::BlockingDataset::Open(lance_uri, storage_options)); ArrowSchema c_schema; ARROW_RETURN_NOT_OK(arrow::ExportSchema(*schema_, &c_schema)); - auto scanner = dataset->Scan(c_schema, 8192); - auto stream = scanner->OpenStream(); + ARROW_ASSIGN_OR_RAISE(auto scanner, dataset->Scan(c_schema, 8192)); + ARROW_ASSIGN_OR_RAISE(auto stream, scanner->OpenStream()); ARROW_ASSIGN_OR_RAISE(auto reader, arrow::ImportRecordBatchReader(&stream)); @@ -612,12 +608,12 @@ BENCHMARK_DEFINE_F(StorageLayerFixture, LanceNative_Take)(::benchmark::State& st // Lambda to take from lance dataset auto take_lance = [&](bool collect_stats, int64_t& out_rows, int64_t& out_bytes) -> arrow::Status { - auto dataset = lance::BlockingDataset::Open(lance_uri, storage_options); + ARROW_ASSIGN_OR_RAISE(auto dataset, lance::BlockingDataset::Open(lance_uri, storage_options)); ArrowSchema c_schema; ARROW_RETURN_NOT_OK(arrow::ExportSchema(*schema_, &c_schema)); - auto stream = dataset->Take(indices, c_schema); + ARROW_ASSIGN_OR_RAISE(auto stream, dataset->Take(indices, c_schema)); ARROW_ASSIGN_OR_RAISE(auto reader, arrow::ImportRecordBatchReader(&stream)); @@ -718,13 +714,13 @@ BENCHMARK_DEFINE_F(StorageLayerFixture, LanceNative_MultiReader)(::benchmark::St for (int i = 0; i < num_readers; ++i) { reader_threads.emplace_back([&, i]() { auto read_all = [&]() -> arrow::Status { - auto dataset = lance::BlockingDataset::Open(lance_uri, storage_options); + ARROW_ASSIGN_OR_RAISE(auto dataset, lance::BlockingDataset::Open(lance_uri, storage_options)); ArrowSchema c_schema; ARROW_RETURN_NOT_OK(arrow::ExportSchema(*schema_, &c_schema)); - auto scanner = dataset->Scan(c_schema, 8192); - auto stream = scanner->OpenStream(); + ARROW_ASSIGN_OR_RAISE(auto scanner, dataset->Scan(c_schema, 8192)); + ARROW_ASSIGN_OR_RAISE(auto stream, scanner->OpenStream()); ARROW_ASSIGN_OR_RAISE(auto reader, arrow::ImportRecordBatchReader(&stream)); diff --git a/cpp/src/format/bridge/rust/.gitignore b/cpp/src/format/bridge/rust/.gitignore index 2288c29c7..829a8f60b 100644 --- a/cpp/src/format/bridge/rust/.gitignore +++ b/cpp/src/format/bridge/rust/.gitignore @@ -10,3 +10,6 @@ build/* #ignore others .DS_Store + +# vendored vortex sources generated by patch_vortex.sh +_vortex_patched/ diff --git a/cpp/src/format/bridge/rust/include/bridge_error.h b/cpp/src/format/bridge/rust/include/bridge_error.h new file mode 100644 index 000000000..ddaf3a20e --- /dev/null +++ b/cpp/src/format/bridge/rust/include/bridge_error.h @@ -0,0 +1,64 @@ +// Copyright 2025 Zilliz +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#pragma once + +#include +#include + +#include +#include + +namespace milvus_storage::bridge { + +// Shared decoding of classified errors coming out of the Rust cxx bridges +// (vortex / lance / iceberg). +// +// The cxx boundary only carries an error as a display string; the Rust side +// (rust/src/bridge_error.rs, vortex's filesystem_c.rs) embeds the +// classification as "__LOON_VORTEX_FFI_ERRCODE__=; message". The +// helpers here parse and strip that marker and rebuild a structured +// arrow::Status: +// * code 12 (LOON_FILE_NOT_FOUND) -> IOError + ENOENT detail +// * ExtendStatusCode values (101-112) -> IOError + ExtendStatusDetail +// * bridge-private codes (>= 1000, never cross the C ABI): +// 1001 data-corrupt -> Status::Invalid (permanent data error) +// 1002 not-supported -> Status::NotImplemented +// * no / unknown marker -> plain IOError (conservative +// non-retriable fallback; never invent retriability) + +// Bridge-private marker codes; keep in sync with rust/src/bridge_error.rs. +inline constexpr int kBridgeErrCodeDataCorrupt = 1001; +inline constexpr int kBridgeErrCodeNotSupported = 1002; + +/// Decode a raw bridge error message (marker stripped) into a structured +/// arrow::Status per the table above. +arrow::Status MakeBridgeErrorStatus(std::string_view message); + +/// Prefix `context` onto `status`'s message, preserving its StatusCode and +/// detail (ExtendStatusDetail / errno). OK statuses pass through. +arrow::Status WithBridgeContext(std::string_view context, const arrow::Status& status); + +/// Translate any status that may carry an (encoded or already-structured) +/// bridge error: already-classified statuses just gain context; otherwise the +/// message is scanned for the marker and rebuilt. +arrow::Status TranslateBridgeStatus(std::string_view context, const arrow::Status& status); + +/// Wrap a RecordBatchReader whose ReadNext/Close surface raw bridge error +/// strings (arrow FFI stringification of Rust stream errors) so mid-scan +/// errors are decoded too. `context` is prefixed onto translated errors. +std::shared_ptr WrapBridgeRecordBatchReader(std::shared_ptr inner, + std::string context); + +} // namespace milvus_storage::bridge diff --git a/cpp/src/format/bridge/rust/include/iceberg_bridge.h b/cpp/src/format/bridge/rust/include/iceberg_bridge.h index c5a1e5d25..7ba0e5ba0 100644 --- a/cpp/src/format/bridge/rust/include/iceberg_bridge.h +++ b/cpp/src/format/bridge/rust/include/iceberg_bridge.h @@ -18,14 +18,15 @@ #include #include #include -#include -namespace milvus_storage::iceberg { +#include -class IcebergException : public std::runtime_error { - public: - explicit IcebergException(const std::string& message) : std::runtime_error(message) {} -}; +// Error model: fallible APIs return arrow::Result. Bridge errors surface as +// plain (non-retriable) IOError for now — iceberg-rust error kinds are not +// yet classified on the Rust side (follow-up); no exception leaves the +// library. + +namespace milvus_storage::iceberg { /// Per-file info returned from PlanFiles struct IcebergFileInfo { @@ -42,9 +43,10 @@ struct IcebergFileInfo { /// @param snapshot_id Which snapshot to scan /// @param storage_options S3/cloud config as key-value pairs /// @return Vector of file info, one per data file in the snapshot -std::vector PlanFiles(const std::string& metadata_location, - int64_t snapshot_id, - const std::unordered_map& storage_options); +arrow::Result> PlanFiles( + const std::string& metadata_location, + int64_t snapshot_id, + const std::unordered_map& storage_options); /// Info returned after creating a test Iceberg table. struct IcebergTestTableInfo { @@ -66,11 +68,12 @@ struct IcebergTestTableInfo { /// intending to read via native `gs://` with SA impersonation; the Rust side /// will byte-rewrite the embedded scheme across every level of the metadata /// tree so iceberg-rust's `plan_files` can traverse it under a `gs://` FileIO. -IcebergTestTableInfo CreateTestTable(const std::string& table_dir, - uint64_t num_rows, - bool with_positional_deletes, - const std::vector& deleted_positions, - const std::unordered_map& storage_options = {}, - const std::string& record_scheme_override = ""); +arrow::Result CreateTestTable( + const std::string& table_dir, + uint64_t num_rows, + bool with_positional_deletes, + const std::vector& deleted_positions, + const std::unordered_map& storage_options = {}, + const std::string& record_scheme_override = ""); } // namespace milvus_storage::iceberg diff --git a/cpp/src/format/bridge/rust/include/lance_bridge.h b/cpp/src/format/bridge/rust/include/lance_bridge.h index a2268586e..156f9c8ca 100644 --- a/cpp/src/format/bridge/rust/include/lance_bridge.h +++ b/cpp/src/format/bridge/rust/include/lance_bridge.h @@ -18,13 +18,21 @@ #include #include #include -#include #include #include +#include #include "rust/cxx.h" #include "rust-bridge/lib.h" +// Error model: every fallible API returns arrow::Result / arrow::Status. +// Errors coming out of the Rust bridge carry a classification marker (see +// bridge_error.h) that is decoded into a structured status here — not-found +// surfaces with an ENOENT detail, transients with an ExtendStatusDetail, +// corruption as Status::Invalid — instead of the previous string-only +// LanceException, which leaked exceptions out of the library and collapsed +// every failure class into one bucket. + namespace milvus_storage::lance { /// Replace the global Lance tokio runtime with a new one using the specified number of worker threads. @@ -39,11 +47,6 @@ namespace milvus_storage::lance { /// Violating any of the above leads to undefined behavior (use-after-free, data races). void ReplaceLanceRuntime(uint32_t num_threads); -class LanceException : public std::runtime_error { - public: - explicit LanceException(const std::string& message) : std::runtime_error(message) {} -}; - class BlockingFragmentReader; class BlockingScanner; @@ -67,19 +70,21 @@ enum class LanceDataStorageFormat : uint8_t { class BlockingDataset { public: - static std::shared_ptr Open(const std::string& uri, const StorageOptions& storage_options = {}); + static arrow::Result> Open(const std::string& uri, + const StorageOptions& storage_options = {}); - static std::unique_ptr OpenUnique(const std::string& uri, - const StorageOptions& storage_options = {}); + static arrow::Result> OpenUnique(const std::string& uri, + const StorageOptions& storage_options = {}); - static std::unique_ptr WriteDataset(const std::string& uri, - struct ArrowArrayStream* stream, - const StorageOptions& storage_options = {}, - LanceDataStorageFormat format = LanceDataStorageFormat::Stable); + static arrow::Result> WriteDataset( + const std::string& uri, + struct ArrowArrayStream* stream, + const StorageOptions& storage_options = {}, + LanceDataStorageFormat format = LanceDataStorageFormat::Stable); explicit BlockingDataset(rust::Box impl) : impl_(std::move(impl)) {} - void WriteArrowArrayStream(struct ArrowArrayStream* stream); + arrow::Status WriteArrowArrayStream(struct ArrowArrayStream* stream); BlockingDataset(BlockingDataset&&) noexcept = default; BlockingDataset& operator=(BlockingDataset&&) noexcept = default; @@ -87,32 +92,34 @@ class BlockingDataset { BlockingDataset(const BlockingDataset&) = delete; BlockingDataset& operator=(const BlockingDataset&) = delete; - void DeleteRows(const std::string& predicate); + arrow::Status DeleteRows(const std::string& predicate); - std::vector GetAllFragmentIds() const; + arrow::Result> GetAllFragmentIds() const; - std::vector GetFragmentDeletionPositions(uint64_t fragment_id) const; + arrow::Result> GetFragmentDeletionPositions(uint64_t fragment_id) const; - uint64_t GetFragmentPhysicalRowCount(uint64_t fragment_id) const; + arrow::Result GetFragmentPhysicalRowCount(uint64_t fragment_id) const; - uint64_t GetFragmentRowCount(uint64_t fragment_id) const; + arrow::Result GetFragmentRowCount(uint64_t fragment_id) const; // Top-level dataset columns in schema order; returns NotImplemented when estimation is unavailable. arrow::Result> EstimateFragmentColumnMemory(uint64_t fragment_id) const; + // Best-effort: returns 0 when estimation is unavailable. uint64_t EstimateFragmentMemory(uint64_t fragment_id) const; // Lance 7 exposes the current dataset schema through FileFragment::schema(). // It can include evolved nullable columns that are not physically stored in this fragment. - void GetFragmentSchema(uint64_t fragment_id, ArrowSchema& out_schema) const; + arrow::Status GetFragmentSchema(uint64_t fragment_id, ArrowSchema& out_schema) const; // Dataset-level scan: create a scanner for projected columns - std::unique_ptr Scan(ArrowSchema& schema, uint32_t batch_size); + arrow::Result> Scan(ArrowSchema& schema, uint32_t batch_size); // Dataset-level take: random access by global row indices - ArrowArrayStream Take(const std::vector& indices, ArrowSchema& schema); + arrow::Result Take(const std::vector& indices, ArrowSchema& schema); /// Read and reset IO statistics for this dataset's object store. + /// Best-effort: returns zeroes when statistics are unavailable. LanceIOStats IOStatsIncremental(); const ffi::BlockingDataset& Impl() const { return *impl_; } @@ -123,9 +130,9 @@ class BlockingDataset { class BlockingFragmentReader { public: - static std::unique_ptr Open(const BlockingDataset& dataset, - uint64_t fragment_id, - ArrowSchema& schema); + static arrow::Result> Open(const BlockingDataset& dataset, + uint64_t fragment_id, + ArrowSchema& schema); explicit BlockingFragmentReader(rust::Box impl) : impl_(std::move(impl)) {} @@ -135,15 +142,17 @@ class BlockingFragmentReader { BlockingFragmentReader(const BlockingFragmentReader&) = delete; BlockingFragmentReader& operator=(const BlockingFragmentReader&) = delete; - uint64_t RowCount() const; + arrow::Result RowCount() const; - void TakeAsSingleBatch(const std::vector& indices, ArrowArray& out_array); + arrow::Status TakeAsSingleBatch(const std::vector& indices, ArrowArray& out_array); - ArrowArrayStream TakeAsStream(const std::vector& indices, uint32_t batch_size); + arrow::Result TakeAsStream(const std::vector& indices, uint32_t batch_size); - ArrowArrayStream ReadAllAsStream(uint32_t batch_size); + arrow::Result ReadAllAsStream(uint32_t batch_size); - ArrowArrayStream ReadRangesAsStream(uint32_t row_range_start, uint32_t row_range_end, uint32_t batch_size); + arrow::Result ReadRangesAsStream(uint32_t row_range_start, + uint32_t row_range_end, + uint32_t batch_size); private: rust::Box impl_; @@ -159,9 +168,9 @@ class BlockingScanner { BlockingScanner(const BlockingScanner&) = delete; BlockingScanner& operator=(const BlockingScanner&) = delete; - uint64_t CountRows() const; + arrow::Result CountRows() const; - ArrowArrayStream OpenStream(); + arrow::Result OpenStream(); private: rust::Box impl_; diff --git a/cpp/src/format/bridge/rust/src/bridge_error.cpp b/cpp/src/format/bridge/rust/src/bridge_error.cpp new file mode 100644 index 000000000..8f646af8c --- /dev/null +++ b/cpp/src/format/bridge/rust/src/bridge_error.cpp @@ -0,0 +1,153 @@ +// Copyright 2025 Zilliz +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#include "bridge_error.h" + +#include +#include +#include +#include +#include + +#include + +#include "milvus-storage/common/extend_status.h" +#include "milvus-storage/ffi_internal/ffi_error_code.h" + +namespace milvus_storage::bridge { +namespace { + +// One marker, one parser: must stay byte-identical to the constants in +// rust/src/bridge_error.rs and rust/src/filesystem_c.rs. +constexpr std::string_view kBridgeErrCodeMarker = "__LOON_VORTEX_FFI_ERRCODE__="; + +struct ParsedBridgeError { + std::string message; + std::optional ffi_err_code; +}; + +std::string StripBridgeMarker(std::string_view error, size_t marker_pos, size_t code_end) { + auto message_start = code_end; + if (message_start < error.size() && error[message_start] == ';') { + ++message_start; + } + if (message_start < error.size() && error[message_start] == ' ') { + ++message_start; + } + + std::string message; + message.reserve(error.size()); + message.append(error.substr(0, marker_pos)); + message.append(error.substr(message_start)); + if (message.empty()) { + return "Unknown bridge error"; + } + return message; +} + +ParsedBridgeError ParseBridgeError(std::string_view error) { + auto marker_pos = error.find(kBridgeErrCodeMarker); + if (marker_pos == std::string_view::npos) { + return {std::string(error), std::nullopt}; + } + + auto code_start = marker_pos + kBridgeErrCodeMarker.size(); + auto code_end = code_start; + while (code_end < error.size() && error[code_end] >= '0' && error[code_end] <= '9') { + ++code_end; + } + if (code_end == code_start) { + return {std::string(error), std::nullopt}; + } + + int ffi_err_code = 0; + auto parse_result = std::from_chars(error.data() + code_start, error.data() + code_end, ffi_err_code); + if (parse_result.ec != std::errc()) { + return {StripBridgeMarker(error, marker_pos, code_end), std::nullopt}; + } + + return {StripBridgeMarker(error, marker_pos, code_end), ffi_err_code}; +} + +class BridgeErrorTranslatingReader final : public arrow::RecordBatchReader { + public: + BridgeErrorTranslatingReader(std::shared_ptr inner, std::string context) + : inner_(std::move(inner)), context_(std::move(context)) {} + + [[nodiscard]] std::shared_ptr schema() const override { return inner_->schema(); } + + arrow::Status ReadNext(std::shared_ptr* batch) override { + return TranslateBridgeStatus(context_, inner_->ReadNext(batch)); + } + + arrow::Status Close() override { return TranslateBridgeStatus(context_, inner_->Close()); } + + private: + std::shared_ptr inner_; + std::string context_; +}; + +} // namespace + +arrow::Status MakeBridgeErrorStatus(std::string_view message) { + auto parsed = ParseBridgeError(message); + if (parsed.ffi_err_code.has_value()) { + switch (*parsed.ffi_err_code) { + case LOON_FILE_NOT_FOUND: + return arrow::Status::IOError(parsed.message).WithDetail(arrow::internal::StatusDetailFromErrno(ENOENT)); + case kBridgeErrCodeDataCorrupt: + return arrow::Status::Invalid(parsed.message); + case kBridgeErrCodeNotSupported: + return arrow::Status::NotImplemented(parsed.message); + default: + break; + } + if (auto code = ExtendStatusCodeFromInt(*parsed.ffi_err_code); code.has_value()) { + return MakeExtendError(*code, parsed.message, parsed.message); + } + } + return arrow::Status::IOError(parsed.message); +} + +arrow::Status WithBridgeContext(std::string_view context, const arrow::Status& status) { + if (status.ok() || context.empty()) { + return status; + } + std::string message; + message.reserve(context.size() + 2 + status.message().size()); + message.append(context); + message.append(": "); + message.append(status.message()); + // Same StatusCode and detail (ExtendStatusDetail / errno) — only the message + // gains context; classification is never altered here. + return {status.code(), std::move(message), status.detail()}; +} + +arrow::Status TranslateBridgeStatus(std::string_view context, const arrow::Status& status) { + if (status.ok()) { + return status; + } + if (ExtendStatusDetail::UnwrapStatus(status) || arrow::internal::ErrnoFromStatus(status) == ENOENT) { + // Already structured — nothing to decode. + return WithBridgeContext(context, status); + } + return WithBridgeContext(context, MakeBridgeErrorStatus(status.message())); +} + +std::shared_ptr WrapBridgeRecordBatchReader(std::shared_ptr inner, + std::string context) { + return std::make_shared(std::move(inner), std::move(context)); +} + +} // namespace milvus_storage::bridge diff --git a/cpp/src/format/bridge/rust/src/bridge_error.rs b/cpp/src/format/bridge/rust/src/bridge_error.rs new file mode 100644 index 000000000..ef4290940 --- /dev/null +++ b/cpp/src/format/bridge/rust/src/bridge_error.rs @@ -0,0 +1,149 @@ +// Copyright 2025 Zilliz +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +//! Shared error classification for the Rust cxx bridges. +//! +//! The cxx boundary can only carry an error as a display string +//! (`rust::Error::what()`), which used to destroy the typed error the Rust +//! side already had (`lance::Error` distinguishes not-found / corruption / +//! retryable contention; the C++ side then guessed a blanket classification). +//! To keep the classification across the string-only channel, an error code is +//! embedded into the message with a marker prefix that the C++ side parses and +//! strips (see cpp `bridge_error.h`), the same mechanism the vortex bridge +//! established in `filesystem_c.rs`. +//! +//! Code space carried by the marker: +//! * LOON / ExtendStatusCode values (`ffi_error_code.h`): 12 = file-not-found, +//! 101-112 = AWS/transient/txn extend codes. The C++ side rebuilds the +//! matching `ExtendStatusDetail` (or an ENOENT detail for 12). +//! * Bridge-private values (>= 1000, never cross the C ABI): the C++ side +//! converts them straight into an arrow StatusCode and they cease to exist. +//! +//! Classification discipline ("producer owns classification", conservative): +//! only signals the producer positively identifies are tagged; everything else +//! stays untagged and lands in the consumer's non-retriable fallback bucket. +//! Never invent retriability. + +use lance::Error as LanceError; + +/// Must stay byte-identical to the vortex marker in `filesystem_c.rs` and the +/// parser constant in cpp `bridge_error.cpp` — one marker, one parser. +pub const BRIDGE_ERRCODE_MARKER: &str = "__LOON_VORTEX_FFI_ERRCODE__="; + +/// Mirrors LOON_FILE_NOT_FOUND in `ffi_error_code.h`. +pub const LOON_FILE_NOT_FOUND: i32 = 12; +/// Mirror of the ExtendStatusCode transient tags (`ffi_error_code.h` 101-112). +pub const LOON_AWS_ERROR_PRECONDITION_FAILED: i32 = 103; +pub const LOON_AWS_ERROR_ACCESS_DENIED: i32 = 105; +pub const LOON_TRANSIENT_THROTTLING: i32 = 109; + +/// Bridge-private codes (>= 1000): decoded by cpp `bridge_error.cpp` into an +/// arrow StatusCode, never forwarded as an FFI error code. +pub const BRIDGE_ERRCODE_DATA_CORRUPT: i32 = 1001; +pub const BRIDGE_ERRCODE_NOT_SUPPORTED: i32 = 1002; + +/// Error type used by the cxx bridge functions. cxx renders it with `Display` +/// into `rust::Error::what()`; the marker survives that trip. +#[derive(Debug)] +pub struct BridgeError { + pub code: Option, + pub msg: String, +} + +impl std::fmt::Display for BridgeError { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self.code { + Some(code) => write!(f, "{BRIDGE_ERRCODE_MARKER}{code}; {}", self.msg), + None => write!(f, "{}", self.msg), + } + } +} + +impl std::error::Error for BridgeError {} + +/// Alias used to switch a whole bridge impl module to classified errors: the +/// `?` operator converts `lance::Error` (and `ArrowError`) via the `From` +/// impls below. +pub type BridgeResult = std::result::Result; + +/// Classify a `lance::Error` into a marker code. `None` = not positively +/// identified -> stays untagged -> conservative non-retriable fallback on the +/// consumer side. +pub fn classify_lance_error(e: &LanceError) -> Option { + match e { + // The object/dataset/index/ref/version is gone. Retrying hits the same + // store and fails identically; consumers can distinguish "data + // missing" from a generic storage failure. + LanceError::NotFound { .. } + | LanceError::DatasetNotFound { .. } + | LanceError::IndexNotFound { .. } + | LanceError::RefNotFound { .. } + | LanceError::VersionNotFound { .. } + | LanceError::FieldNotFound { .. } => Some(LOON_FILE_NOT_FOUND), + // Permanent data problems: retrying re-reads the same bytes. + LanceError::CorruptFile { .. } + | LanceError::SchemaMismatch { .. } + | LanceError::Schema { .. } => Some(BRIDGE_ERRCODE_DATA_CORRUPT), + LanceError::NotSupported { .. } => Some(BRIDGE_ERRCODE_NOT_SUPPORTED), + // Lance itself declares these retryable: the failed attempt is spent, + // but a fresh attempt (new commit round) can succeed. This is the + // producer's own classification, not invented here. + LanceError::RetryableCommitConflict { .. } | LanceError::TooMuchWriteContention { .. } => { + Some(LOON_TRANSIENT_THROTTLING) + } + // IO wraps the underlying object_store error as a boxed source; + // downcast to recover the typed variant. + LanceError::IO { source, .. } => match source.downcast_ref::() { + Some(object_store::Error::NotFound { .. }) => Some(LOON_FILE_NOT_FOUND), + Some( + object_store::Error::PermissionDenied { .. } + | object_store::Error::Unauthenticated { .. }, + ) => Some(LOON_AWS_ERROR_ACCESS_DENIED), + Some(object_store::Error::Precondition { .. }) => { + Some(LOON_AWS_ERROR_PRECONDITION_FAILED) + } + Some( + object_store::Error::NotSupported { .. } + | object_store::Error::NotImplemented { .. }, + ) => Some(BRIDGE_ERRCODE_NOT_SUPPORTED), + // Generic and friends: object_store has already spent its own + // retry budget; no positive transient/permanent signal survives, + // so stay untagged (conservative). + _ => None, + }, + // InvalidInput deliberately NOT tagged as caller input: the strings we + // feed lance are mostly assembled by this library itself, so blaming + // the caller would misroute retries (see the 2007/2020/2021 + // demotions). Left untagged pending a producer-site audit. + _ => None, + } +} + +impl From for BridgeError { + fn from(e: LanceError) -> Self { + BridgeError { + code: classify_lance_error(&e), + msg: e.to_string(), + } + } +} + +impl From for BridgeError { + fn from(e: arrow58::error::ArrowError) -> Self { + BridgeError { + code: None, + msg: e.to_string(), + } + } +} diff --git a/cpp/src/format/bridge/rust/src/iceberg_bridge.cpp b/cpp/src/format/bridge/rust/src/iceberg_bridge.cpp index 0a3b0f905..02a1c1e13 100644 --- a/cpp/src/format/bridge/rust/src/iceberg_bridge.cpp +++ b/cpp/src/format/bridge/rust/src/iceberg_bridge.cpp @@ -13,6 +13,7 @@ // limitations under the License. #include "iceberg_bridge.h" +#include "bridge_error.h" #include "bridge_util.h" #include "rust/cxx.h" @@ -22,9 +23,10 @@ namespace milvus_storage::iceberg { using milvus_storage::ConvertStorageOptions; -std::vector PlanFiles(const std::string& metadata_location, - int64_t snapshot_id, - const std::unordered_map& storage_options) { +arrow::Result> PlanFiles( + const std::string& metadata_location, + int64_t snapshot_id, + const std::unordered_map& storage_options) { try { rust::Vec keys, values; ConvertStorageOptions(storage_options, keys, values); @@ -44,16 +46,16 @@ std::vector PlanFiles(const std::string& metadata_location, } return result; } catch (const rust::cxxbridge1::Error& e) { - throw IcebergException(e.what()); + return milvus_storage::bridge::MakeBridgeErrorStatus(e.what()); } } -IcebergTestTableInfo CreateTestTable(const std::string& table_dir, - uint64_t num_rows, - bool with_positional_deletes, - const std::vector& deleted_positions, - const std::unordered_map& storage_options, - const std::string& record_scheme_override) { +arrow::Result CreateTestTable(const std::string& table_dir, + uint64_t num_rows, + bool with_positional_deletes, + const std::vector& deleted_positions, + const std::unordered_map& storage_options, + const std::string& record_scheme_override) { try { rust::Vec rust_positions; for (auto pos : deleted_positions) { @@ -73,7 +75,7 @@ IcebergTestTableInfo CreateTestTable(const std::string& table_dir, std::string(result.data_file_uri.data(), result.data_file_uri.size()), }; } catch (const rust::cxxbridge1::Error& e) { - throw IcebergException(e.what()); + return milvus_storage::bridge::MakeBridgeErrorStatus(e.what()); } } diff --git a/cpp/src/format/bridge/rust/src/lance_bridge.cpp b/cpp/src/format/bridge/rust/src/lance_bridge.cpp index 338a792e0..c747f8b81 100644 --- a/cpp/src/format/bridge/rust/src/lance_bridge.cpp +++ b/cpp/src/format/bridge/rust/src/lance_bridge.cpp @@ -13,98 +13,109 @@ // limitations under the License. #include "lance_bridge.h" +#include "bridge_error.h" #include "bridge_util.h" #include +#include namespace milvus_storage::lance { void ReplaceLanceRuntime(uint32_t num_threads) {} using milvus_storage::ConvertStorageOptions; +using milvus_storage::bridge::MakeBridgeErrorStatus; -std::shared_ptr BlockingDataset::Open(const std::string& uri, const StorageOptions& storage_options) { +namespace { + +// Run a bridge call, translating the marker-encoded rust::Error into a +// structured arrow error (see bridge_error.h). No exception leaves this +// library. +template +auto CatchRustResult(Fn&& fn) -> arrow::Result { try { + return fn(); + } catch (const rust::cxxbridge1::Error& e) { + return MakeBridgeErrorStatus(e.what()); + } +} + +template +arrow::Status CatchRustStatus(Fn&& fn) { + try { + fn(); + return arrow::Status::OK(); + } catch (const rust::cxxbridge1::Error& e) { + return MakeBridgeErrorStatus(e.what()); + } +} + +} // namespace + +arrow::Result> BlockingDataset::Open(const std::string& uri, + const StorageOptions& storage_options) { + return CatchRustResult([&] { rust::Vec keys, values; ConvertStorageOptions(storage_options, keys, values); return std::make_shared( ffi::open_dataset(rust::Str(uri.data(), uri.length()), std::move(keys), std::move(values))); - } catch (const rust::cxxbridge1::Error& e) { - throw LanceException(e.what()); - } + }); } -std::unique_ptr BlockingDataset::OpenUnique(const std::string& uri, - const StorageOptions& storage_options) { - try { +arrow::Result> BlockingDataset::OpenUnique(const std::string& uri, + const StorageOptions& storage_options) { + return CatchRustResult([&] { rust::Vec keys, values; ConvertStorageOptions(storage_options, keys, values); return std::make_unique( ffi::open_dataset(rust::Str(uri.data(), uri.length()), std::move(keys), std::move(values))); - } catch (const rust::cxxbridge1::Error& e) { - throw LanceException(e.what()); - } + }); } -std::unique_ptr BlockingDataset::WriteDataset(const std::string& uri, - struct ArrowArrayStream* stream, - const StorageOptions& storage_options, - LanceDataStorageFormat format) { - try { +arrow::Result> BlockingDataset::WriteDataset(const std::string& uri, + struct ArrowArrayStream* stream, + const StorageOptions& storage_options, + LanceDataStorageFormat format) { + return CatchRustResult([&] { rust::Vec keys, values; ConvertStorageOptions(storage_options, keys, values); auto ffi_format = static_cast(format); return std::make_unique(ffi::write_dataset(rust::Str(uri.data(), uri.length()), reinterpret_cast(stream), std::move(keys), std::move(values), ffi_format)); - } catch (const rust::cxxbridge1::Error& e) { - throw LanceException(e.what()); - } + }); } -void BlockingDataset::DeleteRows(const std::string& predicate) { - try { - ffi::dataset_delete_rows(*impl_, rust::Str(predicate.data(), predicate.length())); - } catch (const rust::cxxbridge1::Error& e) { - throw LanceException(e.what()); - } +arrow::Status BlockingDataset::DeleteRows(const std::string& predicate) { + return CatchRustStatus([&] { ffi::dataset_delete_rows(*impl_, rust::Str(predicate.data(), predicate.length())); }); } -std::vector BlockingDataset::GetAllFragmentIds() const { - try { +arrow::Result> BlockingDataset::GetAllFragmentIds() const { + return CatchRustResult([&] { auto fragment_ids = impl_->get_all_fragment_ids(); - return {fragment_ids.begin(), fragment_ids.end()}; - } catch (const rust::cxxbridge1::Error& e) { - throw LanceException(e.what()); - } + return std::vector{fragment_ids.begin(), fragment_ids.end()}; + }); } -std::vector BlockingDataset::GetFragmentDeletionPositions(uint64_t fragment_id) const { - try { +arrow::Result> BlockingDataset::GetFragmentDeletionPositions(uint64_t fragment_id) const { + return CatchRustResult([&] { auto positions = ffi::get_fragment_deletion_positions(*impl_, fragment_id); - return {positions.begin(), positions.end()}; - } catch (const rust::cxxbridge1::Error& e) { - throw LanceException(e.what()); - } + return std::vector{positions.begin(), positions.end()}; + }); } -uint64_t BlockingDataset::GetFragmentPhysicalRowCount(uint64_t fragment_id) const { - try { - return ffi::get_fragment_physical_row_count(*impl_, fragment_id); - } catch (const rust::cxxbridge1::Error& e) { - throw LanceException(e.what()); - } +arrow::Result BlockingDataset::GetFragmentPhysicalRowCount(uint64_t fragment_id) const { + return CatchRustResult([&] { return ffi::get_fragment_physical_row_count(*impl_, fragment_id); }); } -uint64_t BlockingDataset::GetFragmentRowCount(uint64_t fragment_id) const { - try { - return ffi::get_fragment_row_count(*impl_, fragment_id); - } catch (const rust::cxxbridge1::Error& e) { - throw LanceException(e.what()); - } +arrow::Result BlockingDataset::GetFragmentRowCount(uint64_t fragment_id) const { + return CatchRustResult([&] { return ffi::get_fragment_row_count(*impl_, fragment_id); }); } arrow::Result> BlockingDataset::EstimateFragmentColumnMemory(uint64_t fragment_id) const { + // Estimation is a best-effort sizing hint: failures degrade to + // NotImplemented so callers fall back to coarser estimates instead of + // failing the open (see the reader-side NotImplemented handling). try { auto estimates = ffi::estimate_fragment_column_memory(*impl_, fragment_id); std::vector memory_sizes; @@ -126,131 +137,102 @@ uint64_t BlockingDataset::EstimateFragmentMemory(uint64_t fragment_id) const { } } -void BlockingDataset::GetFragmentSchema(uint64_t fragment_id, ArrowSchema& out_schema) const { - try { - ffi::get_fragment_schema(*impl_, fragment_id, reinterpret_cast(&out_schema)); - } catch (const rust::cxxbridge1::Error& e) { - throw LanceException(e.what()); - } +arrow::Status BlockingDataset::GetFragmentSchema(uint64_t fragment_id, ArrowSchema& out_schema) const { + return CatchRustStatus( + [&] { ffi::get_fragment_schema(*impl_, fragment_id, reinterpret_cast(&out_schema)); }); } -void BlockingDataset::WriteArrowArrayStream(struct ArrowArrayStream* stream) { - try { - impl_->write_stream(reinterpret_cast(stream)); - } catch (const rust::cxxbridge1::Error& e) { - throw LanceException(e.what()); - } +arrow::Status BlockingDataset::WriteArrowArrayStream(struct ArrowArrayStream* stream) { + return CatchRustStatus([&] { impl_->write_stream(reinterpret_cast(stream)); }); } -std::unique_ptr BlockingFragmentReader::Open(const BlockingDataset& dataset, - uint64_t fragment_id, - ArrowSchema& schema) { - try { +arrow::Result> BlockingFragmentReader::Open(const BlockingDataset& dataset, + uint64_t fragment_id, + ArrowSchema& schema) { + return CatchRustResult([&] { auto impl = ffi::open_fragment_reader(dataset.Impl(), fragment_id, reinterpret_cast(&schema)); return std::make_unique(std::move(impl)); - } catch (const rust::cxxbridge1::Error& e) { - throw LanceException(e.what()); - } + }); } -uint64_t BlockingFragmentReader::RowCount() const { - try { - return impl_->number_of_rows(); - } catch (const rust::cxxbridge1::Error& e) { - throw LanceException(e.what()); - } +arrow::Result BlockingFragmentReader::RowCount() const { + return CatchRustResult([&] { return impl_->number_of_rows(); }); } -void BlockingFragmentReader::TakeAsSingleBatch(const std::vector& indices, ArrowArray& out_array) { - try { +arrow::Status BlockingFragmentReader::TakeAsSingleBatch(const std::vector& indices, ArrowArray& out_array) { + return CatchRustStatus([&] { std::vector uint32_indices(indices.begin(), indices.end()); rust::Slice indices_slice(uint32_indices.data(), uint32_indices.size()); impl_->take_as_single_batch(indices_slice, reinterpret_cast(&out_array)); - } catch (const rust::cxxbridge1::Error& e) { - throw LanceException(e.what()); - } + }); } -ArrowArrayStream BlockingFragmentReader::TakeAsStream(const std::vector& indices, uint32_t batch_size) { - try { +arrow::Result BlockingFragmentReader::TakeAsStream(const std::vector& indices, + uint32_t batch_size) { + return CatchRustResult([&] { ArrowArrayStream stream; std::vector uint32_indices(indices.begin(), indices.end()); rust::Slice indices_slice(uint32_indices.data(), uint32_indices.size()); impl_->take_as_stream(indices_slice, batch_size, reinterpret_cast(&stream)); return stream; - } catch (const rust::cxxbridge1::Error& e) { - throw LanceException(e.what()); - } + }); } -ArrowArrayStream BlockingFragmentReader::ReadAllAsStream(uint32_t batch_size) { - try { +arrow::Result BlockingFragmentReader::ReadAllAsStream(uint32_t batch_size) { + return CatchRustResult([&] { ArrowArrayStream stream; impl_->read_all_as_stream(batch_size, reinterpret_cast(&stream)); return stream; - } catch (const rust::cxxbridge1::Error& e) { - throw LanceException(e.what()); - } + }); } -ArrowArrayStream BlockingFragmentReader::ReadRangesAsStream(uint32_t row_range_start, - uint32_t row_range_end, - uint32_t batch_size) { - try { +arrow::Result BlockingFragmentReader::ReadRangesAsStream(uint32_t row_range_start, + uint32_t row_range_end, + uint32_t batch_size) { + return CatchRustResult([&] { ArrowArrayStream stream; impl_->read_ranges_as_stream(row_range_start, row_range_end, batch_size, reinterpret_cast(&stream)); return stream; - } catch (const rust::cxxbridge1::Error& e) { - throw LanceException(e.what()); - } + }); } -std::unique_ptr BlockingDataset::Scan(ArrowSchema& schema, uint32_t batch_size) { - try { +arrow::Result> BlockingDataset::Scan(ArrowSchema& schema, uint32_t batch_size) { + return CatchRustResult([&] { auto impl = ffi::create_scanner(*impl_, reinterpret_cast(&schema), batch_size); return std::make_unique(std::move(impl)); - } catch (const rust::cxxbridge1::Error& e) { - throw LanceException(e.what()); - } + }); } LanceIOStats BlockingDataset::IOStatsIncremental() { + // Statistics are advisory: never fail a read path over a stats hiccup. try { auto stats = impl_->io_stats_incremental(); return {stats.read_iops, stats.read_bytes}; - } catch (const rust::cxxbridge1::Error& e) { - throw LanceException(e.what()); + } catch (const rust::cxxbridge1::Error&) { + return {}; } } -ArrowArrayStream BlockingDataset::Take(const std::vector& indices, ArrowSchema& schema) { - try { +arrow::Result BlockingDataset::Take(const std::vector& indices, ArrowSchema& schema) { + return CatchRustResult([&] { ArrowArrayStream stream; std::vector uint64_indices(indices.begin(), indices.end()); rust::Slice indices_slice(uint64_indices.data(), uint64_indices.size()); ffi::dataset_take(*impl_, indices_slice, reinterpret_cast(&schema), reinterpret_cast(&stream)); return stream; - } catch (const rust::cxxbridge1::Error& e) { - throw LanceException(e.what()); - } + }); } -uint64_t BlockingScanner::CountRows() const { - try { - return impl_->count_rows(); - } catch (const rust::cxxbridge1::Error& e) { - throw LanceException(e.what()); - } +arrow::Result BlockingScanner::CountRows() const { + return CatchRustResult([&] { return impl_->count_rows(); }); } -ArrowArrayStream BlockingScanner::OpenStream() { - try { +arrow::Result BlockingScanner::OpenStream() { + return CatchRustResult([&] { ArrowArrayStream stream; impl_->open_stream(reinterpret_cast(&stream)); return stream; - } catch (const rust::cxxbridge1::Error& e) { - throw LanceException(e.what()); - } + }); } } // namespace milvus_storage::lance diff --git a/cpp/src/format/bridge/rust/src/lance_bridgeimpl.rs b/cpp/src/format/bridge/rust/src/lance_bridgeimpl.rs index e2c821bd4..2d10247f0 100644 --- a/cpp/src/format/bridge/rust/src/lance_bridgeimpl.rs +++ b/cpp/src/format/bridge/rust/src/lance_bridgeimpl.rs @@ -19,8 +19,9 @@ use arrow58::datatypes::SchemaRef; use arrow58::error::ArrowError; use arrow58::ffi_stream::{ArrowArrayStreamReader, FFI_ArrowArrayStream}; -use lance::dataset::builder::DatasetBuilder; +use lance::Error as LanceError; use lance::dataset::AutoCleanupParams; +use lance::dataset::builder::DatasetBuilder; use lance::dataset::cleanup::{CleanupPolicy, RemovalStats}; use lance::dataset::fragment::{FileFragment, FragReadConfig, FragmentReader}; use lance::dataset::optimize::{CompactionOptions as RustCompactionOptions, compact_files}; @@ -29,7 +30,11 @@ use lance::dataset::scanner::Scanner; use lance::dataset::statistics::{DataStatistics, DatasetStatisticsExt}; use lance::dataset::transaction::{Operation, Transaction}; use lance::dataset::{CommitBuilder, Dataset, ReadParams, Version, WriteMode, WriteParams}; -use lance::{Error as LanceError, Result}; + +// Bridge functions report classified errors: BridgeResult's `?` converts +// lance::Error via classify_lance_error, embedding the classification marker +// that survives the string-only cxx boundary (see bridge_error.rs). +use crate::bridge_error::{BridgeError, BridgeResult as Result}; use lance_encoding::version::LanceFileVersion; use crate::lance_ffi::{LanceColumnMemoryEstimate, LanceDataStorageFormat}; @@ -113,9 +118,9 @@ impl BlockingDataset { ) -> Result { let mut store_params = ObjectStoreParams { block_size: block_size.map(|size| size as usize), - storage_options_accessor: Some(Arc::new( - StorageOptionsAccessor::with_static_options(storage_options.clone()), - )), + storage_options_accessor: Some(Arc::new(StorageOptionsAccessor::with_static_options( + storage_options.clone(), + ))), ..Default::default() }; if let Some(offset_seconds) = s3_credentials_refresh_offset_seconds { @@ -409,12 +414,10 @@ impl AssumeRoleConfig { return Ok(None); } if credential_refresh_secs < 900 || credential_refresh_secs > 43200 { - return Err(LanceError::invalid_input( - format!( - "credential_refresh_secs must be in [900, 43200], got {}", - credential_refresh_secs - ), - )); + return Err(BridgeError::from(LanceError::invalid_input(format!( + "credential_refresh_secs must be in [900, 43200], got {}", + credential_refresh_secs + )))); } Ok(Some(Self { role_arn: role_arn.to_string(), @@ -496,12 +499,10 @@ impl GcpImpersonationConfig { .and_then(|s| s.parse().ok()) .unwrap_or(0); if token_lifetime_secs < 900 || token_lifetime_secs > 3600 { - return Err(LanceError::invalid_input( - format!( - "gcp_credential_refresh_secs must be in [900, 3600], got {}", - token_lifetime_secs - ), - )); + return Err(BridgeError::from(LanceError::invalid_input(format!( + "gcp_credential_refresh_secs must be in [900, 3600], got {}", + token_lifetime_secs + )))); } Ok(Some(Self { target_sa, @@ -556,12 +557,23 @@ pub fn open_dataset( // Extract ARN fields from storage_options (set by lance::ToStorageOptions on the C++ side) let role_arn = storage_options.remove("aws_role_arn").unwrap_or_default(); - let session_name = storage_options.remove("aws_session_name").unwrap_or_default(); - let external_id = storage_options.remove("aws_external_id").unwrap_or_default(); - let refresh_secs_str = storage_options.remove("aws_credential_refresh_secs").unwrap_or_default(); + let session_name = storage_options + .remove("aws_session_name") + .unwrap_or_default(); + let external_id = storage_options + .remove("aws_external_id") + .unwrap_or_default(); + let refresh_secs_str = storage_options + .remove("aws_credential_refresh_secs") + .unwrap_or_default(); let credential_refresh_secs: u64 = refresh_secs_str.parse().unwrap_or(0); - let assume_role = AssumeRoleConfig::parse(&role_arn, &session_name, &external_id, credential_refresh_secs)?; + let assume_role = AssumeRoleConfig::parse( + &role_arn, + &session_name, + &external_id, + credential_refresh_secs, + )?; let aws_creds = match &assume_role { Some(config) => Some(TOKIO_RT.block_on(config.build_credentials())?), @@ -583,7 +595,16 @@ pub fn open_dataset( // Passing the full session TTL (e.g. 900s) as the offset would cause Lance to // consider credentials expired immediately after issuance (credential thrashing). let ds = BlockingDataset::open( - uri, None, None, 0, 0, storage_options, None, aws_creds, None, custom_session, + uri, + None, + None, + 0, + 0, + storage_options, + None, + aws_creds, + None, + custom_session, )?; Ok(Box::new(ds)) } @@ -625,9 +646,9 @@ pub unsafe fn write_dataset( ..Default::default() }; write_params.store_params = Some(ObjectStoreParams { - storage_options_accessor: Some(Arc::new( - StorageOptionsAccessor::with_static_options(storage_options), - )), + storage_options_accessor: Some(Arc::new(StorageOptionsAccessor::with_static_options( + storage_options, + ))), ..Default::default() }); @@ -649,8 +670,12 @@ impl Iterator for BatchFutStreamReader { self.runtime_handle .block_on(async { self.stream.next().await }) .map(|res| { - // Convert Lance Error to Arrow Error - res.map_err(|e| ArrowError::from_external_error(Box::new(e))) + // Convert Lance Error to Arrow Error. Wrap in BridgeError so the + // classification marker survives the arrow-rs FFI stringification: + // this is the ONLY place mid-scan read errors (the hot transient + // case: S3 throttle/timeout during stream iteration) pass through + // before reaching C++ as an opaque stream error string. + res.map_err(|e| ArrowError::from_external_error(Box::new(BridgeError::from(e)))) }) } } @@ -697,7 +722,11 @@ pub async fn collect_stream_to_batches( stream: ReadBatchFutStream, concurrency: usize, ) -> Result> { - stream.buffered(concurrency).try_collect::>().await + stream + .buffered(concurrency) + .try_collect::>() + .await + .map_err(BridgeError::from) } #[derive(Clone)] @@ -731,7 +760,8 @@ impl BlockingFragmentReader { let dv = TOKIO_RT.block_on(fragment.get_deletion_vector())?; match dv { Some(dv) => { - let mut dels: Vec = dv.as_ref().clone().into_iter().map(|i| i as u32).collect(); + let mut dels: Vec = + dv.as_ref().clone().into_iter().map(|i| i as u32).collect(); dels.sort(); dels } @@ -740,11 +770,8 @@ impl BlockingFragmentReader { }; let meta_schema = fragment.schema(); - let meta_columns: std::collections::HashSet<_> = meta_schema - .fields - .iter() - .map(|f| f.name.clone()) - .collect(); + let meta_columns: std::collections::HashSet<_> = + meta_schema.fields.iter().map(|f| f.name.clone()).collect(); let columns: Vec<_> = arrow_projection .fields() @@ -754,7 +781,8 @@ impl BlockingFragmentReader { .map(|n| n.clone()) .collect(); - let fragment_reader = TOKIO_RT.block_on(fragment.open(&meta_schema.project(&columns)?, read_config))?; + let fragment_reader = + TOKIO_RT.block_on(fragment.open(&meta_schema.project(&columns)?, read_config))?; Ok(Self { inner: fragment_reader, @@ -785,7 +813,10 @@ impl BlockingFragmentReader { if self.sorted_deletions.is_empty() { return logical_indices.to_vec(); } - logical_indices.iter().map(|&i| self.logical_to_physical(i)).collect() + logical_indices + .iter() + .map(|&i| self.logical_to_physical(i)) + .collect() } pub fn number_of_rows(&self) -> Result { @@ -811,12 +842,11 @@ impl BlockingFragmentReader { out_stream: *mut u8, ) -> Result<()> { let physical_indices = self.map_logical_indices(indices); - let read_batch_fut_stream = TOKIO_RT.block_on(self.inner.take(&physical_indices, batch_size, None)); + let read_batch_fut_stream = + TOKIO_RT.block_on(self.inner.take(&physical_indices, batch_size, None)); - let ffi_stream = read_batch_fut_stream?.to_ffi_stream( - Arc::new(self.projection.clone()), - TOKIO_RT.handle().clone(), - ); + let ffi_stream = read_batch_fut_stream? + .to_ffi_stream(Arc::new(self.projection.clone()), TOKIO_RT.handle().clone()); let out_stream = out_stream as *mut FFI_ArrowArrayStream; // # Safety // Arrow C stream interface @@ -827,10 +857,8 @@ impl BlockingFragmentReader { pub unsafe fn read_all_as_stream(&self, batch_size: u32, out_stream: *mut u8) -> Result<()> { let read_batch_fut_stream = TOKIO_RT.block_on(self.inner.read_all(batch_size))?; - let ffi_stream = read_batch_fut_stream.to_ffi_stream( - Arc::new(self.projection.clone()), - TOKIO_RT.handle().clone(), - ); + let ffi_stream = read_batch_fut_stream + .to_ffi_stream(Arc::new(self.projection.clone()), TOKIO_RT.handle().clone()); let out_stream = out_stream as *mut FFI_ArrowArrayStream; unsafe { std::ptr::write(out_stream, ffi_stream) }; Ok(()) @@ -844,10 +872,8 @@ impl BlockingFragmentReader { ) -> Result<()> { let read_batch_fut_stream = TOKIO_RT.block_on(self.inner.read_range(range, batch_size))?; - let ffi_stream = read_batch_fut_stream.to_ffi_stream( - Arc::new(self.projection.clone()), - TOKIO_RT.handle().clone(), - ); + let ffi_stream = read_batch_fut_stream + .to_ffi_stream(Arc::new(self.projection.clone()), TOKIO_RT.handle().clone()); let out_stream = out_stream as *mut FFI_ArrowArrayStream; unsafe { std::ptr::write(out_stream, ffi_stream) }; Ok(()) @@ -886,9 +912,7 @@ pub unsafe fn open_fragment_reader( })?; let ffi_schema = unsafe { - arrow58::ffi::FFI_ArrowSchema::from_raw( - schema_rawptr as *mut arrow58::ffi::FFI_ArrowSchema, - ) + arrow58::ffi::FFI_ArrowSchema::from_raw(schema_rawptr as *mut arrow58::ffi::FFI_ArrowSchema) }; let arrow_schema = ArrowSchema::try_from(&ffi_schema).map_err(|e| LanceError::InvalidInput { @@ -910,18 +934,23 @@ pub fn dataset_delete_rows(dataset: &mut BlockingDataset, predicate: &str) -> Re } /// Get sorted deletion positions for a fragment. Returns empty vec if no deletions. -pub fn get_fragment_deletion_positions(dataset: &BlockingDataset, fragment_id: u64) -> Result> { - let fragment_meta = dataset - .get_fragment(fragment_id) - .ok_or_else(|| LanceError::InvalidInput { - source: format!("Fragment {} not found", fragment_id).into(), - location: snafu::location!(), - })?; +pub fn get_fragment_deletion_positions( + dataset: &BlockingDataset, + fragment_id: u64, +) -> Result> { + let fragment_meta = + dataset + .get_fragment(fragment_id) + .ok_or_else(|| LanceError::InvalidInput { + source: format!("Fragment {} not found", fragment_id).into(), + location: snafu::location!(), + })?; let fragment = FileFragment::new(Arc::new(dataset.inner.clone()), fragment_meta); let dv = TOKIO_RT.block_on(fragment.get_deletion_vector())?; match dv { Some(dv) => { - let mut positions: Vec = dv.as_ref().clone().into_iter().map(|i| i as u64).collect(); + let mut positions: Vec = + dv.as_ref().clone().into_iter().map(|i| i as u64).collect(); positions.sort(); Ok(positions) } @@ -936,13 +965,12 @@ pub fn get_fragment_physical_row_count(dataset: &BlockingDataset, fragment_id: u source: format!("Fragment {} not found", fragment_id).into(), location: snafu::location!(), })?; - fragment - .physical_rows - .map(|n| n as u64) - .ok_or_else(|| LanceError::InvalidInput { + fragment.physical_rows.map(|n| n as u64).ok_or_else(|| { + BridgeError::from(LanceError::InvalidInput { source: format!("Fragment {} has no physical_rows metadata", fragment_id).into(), location: snafu::location!(), }) + }) } pub fn get_fragment_row_count(dataset: &BlockingDataset, fragment_id: u64) -> Result { @@ -952,13 +980,12 @@ pub fn get_fragment_row_count(dataset: &BlockingDataset, fragment_id: u64) -> Re source: format!("Fragment {} not found", fragment_id).into(), location: snafu::location!(), })?; - fragment - .num_rows() - .map(|n| n as u64) - .ok_or_else(|| LanceError::InvalidInput { + fragment.num_rows().map(|n| n as u64).ok_or_else(|| { + BridgeError::from(LanceError::InvalidInput { source: format!("Fragment {} has no row count metadata", fragment_id).into(), location: snafu::location!(), }) + }) } fn estimate_fragment_columns( @@ -987,13 +1014,15 @@ fn estimate_fragment_columns( .fragment_read_config(FragReadConfig::default()) .scan_scheduler .expect("fragment_read_config always installs a scheduler"); - TOKIO_RT.block_on( - crate::lance_memory_estimator::estimate_fragment_column_memory( - &dataset.inner, - &fragment, - scheduler, - ), - ) + TOKIO_RT + .block_on( + crate::lance_memory_estimator::estimate_fragment_column_memory( + &dataset.inner, + &fragment, + scheduler, + ), + ) + .map_err(BridgeError::from) } /// Estimate each top-level column's decoded Arrow buffer size in schema order. @@ -1024,12 +1053,13 @@ pub unsafe fn get_fragment_schema( fragment_id: u64, out_schema_ptr: *mut u8, ) -> Result<()> { - let fragment_meta = dataset - .get_fragment(fragment_id) - .ok_or_else(|| LanceError::InvalidInput { - source: format!("Fragment {} not found", fragment_id).into(), - location: snafu::location!(), - })?; + let fragment_meta = + dataset + .get_fragment(fragment_id) + .ok_or_else(|| LanceError::InvalidInput { + source: format!("Fragment {} not found", fragment_id).into(), + location: snafu::location!(), + })?; // In Lance 7, FileFragment::schema() returns the current dataset schema. It // includes evolved nullable fields that may not be physically stored in this @@ -1039,11 +1069,12 @@ pub unsafe fn get_fragment_schema( let lance_schema = file_fragment.schema(); let arrow_schema: ArrowSchema = lance_schema.into(); - let ffi_schema = arrow58::ffi::FFI_ArrowSchema::try_from(&arrow_schema) - .map_err(|e| LanceError::InvalidInput { + let ffi_schema = arrow58::ffi::FFI_ArrowSchema::try_from(&arrow_schema).map_err(|e| { + LanceError::InvalidInput { source: format!("Failed to export fragment schema: {}", e).into(), location: snafu::location!(), - })?; + } + })?; let out_ptr = out_schema_ptr as *mut arrow58::ffi::FFI_ArrowSchema; unsafe { std::ptr::write(out_ptr, ffi_schema) }; @@ -1105,9 +1136,7 @@ pub unsafe fn create_scanner( batch_size: u32, ) -> Result> { let ffi_schema = unsafe { - arrow58::ffi::FFI_ArrowSchema::from_raw( - schema_ptr as *mut arrow58::ffi::FFI_ArrowSchema, - ) + arrow58::ffi::FFI_ArrowSchema::from_raw(schema_ptr as *mut arrow58::ffi::FFI_ArrowSchema) }; let arrow_schema = ArrowSchema::try_from(&ffi_schema).map_err(|e| LanceError::InvalidInput { @@ -1138,9 +1167,7 @@ pub unsafe fn dataset_take( out_stream: *mut u8, ) -> Result<()> { let ffi_schema = unsafe { - arrow58::ffi::FFI_ArrowSchema::from_raw( - schema_ptr as *mut arrow58::ffi::FFI_ArrowSchema, - ) + arrow58::ffi::FFI_ArrowSchema::from_raw(schema_ptr as *mut arrow58::ffi::FFI_ArrowSchema) }; let arrow_schema = ArrowSchema::try_from(&ffi_schema).map_err(|e| LanceError::InvalidInput { diff --git a/cpp/src/format/bridge/rust/src/lib.rs b/cpp/src/format/bridge/rust/src/lib.rs index 295301877..06554467c 100644 --- a/cpp/src/format/bridge/rust/src/lib.rs +++ b/cpp/src/format/bridge/rust/src/lib.rs @@ -13,6 +13,7 @@ // limitations under the License. mod aliyun_oss_provider; +mod bridge_error; mod gcp_impersonation; mod iceberg_bridgeimpl; mod iceberg_testutil; @@ -123,10 +124,8 @@ pub mod lance_ffi { dataset: &BlockingDataset, fragment_id: u64, ) -> Result>; - pub fn estimate_fragment_memory( - dataset: &BlockingDataset, - fragment_id: u64, - ) -> Result; + pub fn estimate_fragment_memory(dataset: &BlockingDataset, fragment_id: u64) + -> Result; pub unsafe fn get_fragment_schema( dataset: &BlockingDataset, fragment_id: u64, diff --git a/cpp/src/format/bridge/rust/src/vortex_bridge.cpp b/cpp/src/format/bridge/rust/src/vortex_bridge.cpp index e89c59674..01298a895 100644 --- a/cpp/src/format/bridge/rust/src/vortex_bridge.cpp +++ b/cpp/src/format/bridge/rust/src/vortex_bridge.cpp @@ -3,118 +3,18 @@ #include "vortex_bridge.h" -#include -#include -#include #include #include #include -#include +#include "bridge_error.h" #include "milvus-storage/common/extend_status.h" #include "milvus-storage/ffi_c.h" namespace milvus_storage::vortex { namespace { -constexpr std::string_view kVortexFfiErrCodeMarker = "__LOON_VORTEX_FFI_ERRCODE__="; - -std::string StripBridgeMarker(std::string_view error, size_t marker_pos, size_t code_end) { - auto message_start = code_end; - if (message_start < error.size() && error[message_start] == ';') { - ++message_start; - } - if (message_start < error.size() && error[message_start] == ' ') { - ++message_start; - } - - std::string message; - message.reserve(error.size()); - message.append(error.substr(0, marker_pos)); - message.append(error.substr(message_start)); - if (message.empty()) { - return "Unknown Vortex error"; - } - return message; -} - -struct ParsedVortexBridgeError { - std::string message; - std::optional ffi_err_code; -}; - -class VortexErrorTranslatingReader final : public arrow::RecordBatchReader { - public: - explicit VortexErrorTranslatingReader(std::shared_ptr inner) : inner_(std::move(inner)) {} - - std::shared_ptr schema() const override { return inner_->schema(); } - - arrow::Status ReadNext(std::shared_ptr* batch) override { - return MakeVortexErrorStatus("Failed to read vortex record batch", inner_->ReadNext(batch)); - } - - arrow::Status Close() override { - return MakeVortexErrorStatus("Failed to close vortex record batch reader", inner_->Close()); - } - - private: - std::shared_ptr inner_; -}; - -ParsedVortexBridgeError ParseVortexBridgeError(std::string_view error) { - auto marker_pos = error.find(kVortexFfiErrCodeMarker); - if (marker_pos == std::string_view::npos) { - return {std::string(error), std::nullopt}; - } - - auto code_start = marker_pos + kVortexFfiErrCodeMarker.size(); - auto code_end = code_start; - while (code_end < error.size() && error[code_end] >= '0' && error[code_end] <= '9') { - ++code_end; - } - if (code_end == code_start) { - return {std::string(error), std::nullopt}; - } - - int ffi_err_code = 0; - auto parse_result = std::from_chars(error.data() + code_start, error.data() + code_end, ffi_err_code); - if (parse_result.ec != std::errc()) { - return {std::string(error), std::nullopt}; - } - - return {StripBridgeMarker(error, marker_pos, code_end), ffi_err_code}; -} - -std::string JoinContextAndMessage(std::string_view context, std::string_view message) { - if (context.empty()) { - return std::string(message); - } - if (message.empty()) { - return std::string(context); - } - std::string result; - result.reserve(context.size() + 2 + message.size()); - result.append(context); - result.append(": "); - result.append(message); - return result; -} - -arrow::Status MakeExtendErrorWithContext(std::string_view context, const arrow::Status& status) { - auto detail = ExtendStatusDetail::UnwrapStatus(status); - auto full_message = JoinContextAndMessage(context, status.message()); - return MakeExtendError(detail->code(), full_message, full_message); -} - -arrow::Status MakeIOErrorWithContext(std::string_view context, const arrow::Status& status) { - auto result = arrow::Status::IOError(JoinContextAndMessage(context, status.message())); - if (arrow::internal::ErrnoFromStatus(status) == ENOENT) { - return result.WithDetail(arrow::internal::StatusDetailFromErrno(ENOENT)); - } - return result; -} - template arrow::Result CatchRustResult(Fn&& fn) { try { @@ -136,44 +36,23 @@ arrow::Status CatchRustStatus(Fn&& fn) { } // namespace +// Thin delegates over the shared bridge decoder (bridge_error.h); kept so +// existing vortex call sites and tests are unaffected. arrow::Status MakeVortexBridgeErrorStatus(std::string_view message) { - auto parsed = ParseVortexBridgeError(message); - if (parsed.ffi_err_code.has_value()) { - if (*parsed.ffi_err_code == LOON_FILE_NOT_FOUND) { - return arrow::Status::IOError(parsed.message).WithDetail(arrow::internal::StatusDetailFromErrno(ENOENT)); - } - if (auto code = ExtendStatusCodeFromInt(*parsed.ffi_err_code); code.has_value()) { - return MakeExtendError(*code, parsed.message, parsed.message); - } - } - return arrow::Status::IOError(parsed.message); + return milvus_storage::bridge::MakeBridgeErrorStatus(message); } arrow::Status MakeVortexErrorStatus(std::string_view context, std::string_view message) { - return MakeVortexErrorStatus(context, MakeVortexBridgeErrorStatus(message)); + return milvus_storage::bridge::TranslateBridgeStatus(context, milvus_storage::bridge::MakeBridgeErrorStatus(message)); } arrow::Status MakeVortexErrorStatus(std::string_view context, const arrow::Status& status) { - if (status.ok()) { - return arrow::Status::OK(); - } - if (ExtendStatusDetail::UnwrapStatus(status)) { - return MakeExtendErrorWithContext(context, status); - } - if (arrow::internal::ErrnoFromStatus(status) == ENOENT) { - return MakeIOErrorWithContext(context, status); - } - auto message = status.message(); - auto parsed_status = MakeVortexBridgeErrorStatus(message); - if (ExtendStatusDetail::UnwrapStatus(parsed_status)) { - return MakeExtendErrorWithContext(context, parsed_status); - } - return MakeIOErrorWithContext(context, parsed_status); + return milvus_storage::bridge::TranslateBridgeStatus(context, status); } namespace internal { std::shared_ptr WrapVortexRecordBatchReader(std::shared_ptr inner) { - return std::make_shared(std::move(inner)); + return milvus_storage::bridge::WrapBridgeRecordBatchReader(std::move(inner), "Failed to read vortex record batch"); } } // namespace internal diff --git a/cpp/src/format/iceberg/iceberg_format.cpp b/cpp/src/format/iceberg/iceberg_format.cpp index 72335008d..7030e4ce9 100644 --- a/cpp/src/format/iceberg/iceberg_format.cpp +++ b/cpp/src/format/iceberg/iceberg_format.cpp @@ -35,7 +35,7 @@ arrow::Result> IcebergFormat::explore(const st ARROW_ASSIGN_OR_RAISE(auto parsed_uri, StorageUri::Parse(explore_dir)); ARROW_ASSIGN_OR_RAISE(auto iceberg_uri, StorageUri::Make(parsed_uri, false)); - auto file_infos = iceberg::PlanFiles(iceberg_uri, snapshot_id, storage_options); + ARROW_ASSIGN_OR_RAISE(auto file_infos, iceberg::PlanFiles(iceberg_uri, snapshot_id, storage_options)); std::vector files; files.reserve(file_infos.size()); diff --git a/cpp/src/format/lance/lance_format.cpp b/cpp/src/format/lance/lance_format.cpp index 3e6e790ec..0a844d506 100644 --- a/cpp/src/format/lance/lance_format.cpp +++ b/cpp/src/format/lance/lance_format.cpp @@ -33,12 +33,12 @@ arrow::Result> LanceFormat::explore(const std: ARROW_ASSIGN_OR_RAISE(auto lance_base_uri, lance::BuildLanceBaseUri(fs_config, explore_uri.key)); auto storage_options = lance::ToStorageOptions(fs_config); - auto dataset = lance::BlockingDataset::Open(lance_base_uri, storage_options); - auto fragment_ids = dataset->GetAllFragmentIds(); + ARROW_ASSIGN_OR_RAISE(auto dataset, lance::BlockingDataset::Open(lance_base_uri, storage_options)); + ARROW_ASSIGN_OR_RAISE(auto fragment_ids, dataset->GetAllFragmentIds()); std::vector files; for (auto frag_id : fragment_ids) { - auto row_count = dataset->GetFragmentRowCount(frag_id); + ARROW_ASSIGN_OR_RAISE(auto row_count, dataset->GetFragmentRowCount(frag_id)); // Store Milvus-format URI (scheme://address/bucket/key) so the reader // can resolve the right extfs..* by address+bucket. The reader // strips address back to standard form before handing to Lance. diff --git a/cpp/src/format/lance/lance_table_reader.cpp b/cpp/src/format/lance/lance_table_reader.cpp index f11e9e5e5..09d243cf2 100644 --- a/cpp/src/format/lance/lance_table_reader.cpp +++ b/cpp/src/format/lance/lance_table_reader.cpp @@ -31,6 +31,7 @@ #include #include +#include "bridge_error.h" #include "milvus-storage/common/fiu_local.h" #include "milvus-storage/common/log.h" #include "milvus-storage/filesystem/fs.h" @@ -182,32 +183,18 @@ arrow::Result LanceTableReader::MetaTr ARROW_ASSIGN_OR_RAISE(auto fs_config, FilesystemCache::resolve_config(properties, base_uri)); auto lance_uri = ToStandardLanceUri(base_uri); - std::shared_ptr dataset; - try { - dataset = BlockingDataset::Open(lance_uri, ToStorageOptions(fs_config)); - } catch (const std::exception& e) { - return arrow::Status::IOError("Failed to open Lance dataset for metadata: ", e.what()); - } + ARROW_ASSIGN_OR_RAISE(std::shared_ptr dataset, + BlockingDataset::Open(lance_uri, ToStorageOptions(fs_config))); std::shared_ptr file_schema; { ArrowSchema c_fragment_schema; - try { - dataset->GetFragmentSchema(fragment_id, c_fragment_schema); - } catch (const LanceException& e) { - return arrow::Status::IOError(fmt::format("Failed to get fragment schema: {}", e.what())); - } + ARROW_RETURN_NOT_OK(dataset->GetFragmentSchema(fragment_id, c_fragment_schema)); ARROW_ASSIGN_OR_RAISE(file_schema, arrow::ImportSchema(&c_fragment_schema)); } - uint64_t logical_rows = 0; - uint64_t physical_rows = 0; - try { - logical_rows = dataset->GetFragmentRowCount(fragment_id); - physical_rows = dataset->GetFragmentPhysicalRowCount(fragment_id); - } catch (const LanceException& e) { - return arrow::Status::IOError("Failed to get row counts for Lance fragment ", fragment_id, ": ", e.what()); - } + ARROW_ASSIGN_OR_RAISE(uint64_t logical_rows, dataset->GetFragmentRowCount(fragment_id)); + ARROW_ASSIGN_OR_RAISE(uint64_t physical_rows, dataset->GetFragmentPhysicalRowCount(fragment_id)); if (physical_rows < logical_rows) { return arrow::Status::Invalid("Fragment ", fragment_id, " has inconsistent metadata: physical_rows (", physical_rows, ") < logical_rows (", logical_rows, ")"); @@ -288,16 +275,17 @@ arrow::Result> LanceTableReader::MetaTrait::cr ArrowSchema c_arrow_schema; ARROW_RETURN_NOT_OK(arrow::ExportSchema(*requested_schema, &c_arrow_schema)); - try { - reader->fragment_reader_ = - BlockingFragmentReader::Open(*metadata->payload.dataset, metadata->payload.fragment_id, c_arrow_schema); - } catch (const LanceException& e) { + auto fragment_reader_result = + BlockingFragmentReader::Open(*metadata->payload.dataset, metadata->payload.fragment_id, c_arrow_schema); + if (!fragment_reader_result.ok()) { if (c_arrow_schema.release) { c_arrow_schema.release(&c_arrow_schema); } - return arrow::Status::IOError("Failed to open Lance fragment reader for fragment ", metadata->payload.fragment_id, - ": ", e.what()); + return milvus_storage::bridge::WithBridgeContext( + fmt::format("Failed to open Lance fragment reader for fragment {}", metadata->payload.fragment_id), + fragment_reader_result.status()); } + reader->fragment_reader_ = std::move(*fragment_reader_result); return reader; } @@ -316,17 +304,13 @@ arrow::Status LanceTableReader::open() { << ", role_arn=" << (fs_config.role_arn.empty() ? "(empty)" : fs_config.role_arn) << ", external_id_set=" << (fs_config.external_id.empty() ? "false" : "true") << ", use_iam=" << fs_config.use_iam; - dataset_ = BlockingDataset::Open(lance_uri, ToStorageOptions(fs_config)); + ARROW_ASSIGN_OR_RAISE(dataset_, BlockingDataset::Open(lance_uri, ToStorageOptions(fs_config))); } // Lance 7 exposes the current dataset schema through FileFragment::schema(). { ArrowSchema c_fragment_schema; - try { - dataset_->GetFragmentSchema(fragment_id_, c_fragment_schema); - } catch (const LanceException& e) { - return arrow::Status::IOError(fmt::format("Failed to get fragment schema: {}", e.what())); - } + ARROW_RETURN_NOT_OK(dataset_->GetFragmentSchema(fragment_id_, c_fragment_schema)); ARROW_ASSIGN_OR_RAISE(file_schema_, arrow::ImportSchema(&c_fragment_schema)); } @@ -339,23 +323,32 @@ arrow::Status LanceTableReader::open() { ArrowSchema c_arrow_schema; ARROW_RETURN_NOT_OK(arrow::ExportSchema(*read_schema, &c_arrow_schema)); - fragment_reader_ = BlockingFragmentReader::Open(*dataset_, fragment_id_, c_arrow_schema); + { + auto fragment_reader_result = BlockingFragmentReader::Open(*dataset_, fragment_id_, c_arrow_schema); + if (!fragment_reader_result.ok()) { + if (c_arrow_schema.release) { + c_arrow_schema.release(&c_arrow_schema); + } + return milvus_storage::bridge::WithBridgeContext( + fmt::format("Failed to open Lance fragment reader for fragment {}", fragment_id_), + fragment_reader_result.status()); + } + fragment_reader_ = std::move(*fragment_reader_result); + } // Lance's read_range accepts logical indices (post-deletion) and internally // patches the range to skip deleted rows. So row_group_infos uses logical row count. // However, read_range's batch_size is applied to the *physical* range after // patch_range_for_deletions, so we add num_deletions_ to batch_size to ensure // each read produces a single output batch. - auto logical_rows = fragment_reader_->RowCount(); - try { - auto physical_rows = dataset_->GetFragmentPhysicalRowCount(fragment_id_); + ARROW_ASSIGN_OR_RAISE(uint64_t logical_rows, fragment_reader_->RowCount()); + { + ARROW_ASSIGN_OR_RAISE(uint64_t physical_rows, dataset_->GetFragmentPhysicalRowCount(fragment_id_)); if (physical_rows < logical_rows) { return arrow::Status::Invalid("Fragment ", fragment_id_, " has inconsistent metadata: physical_rows (", physical_rows, ") < logical_rows (", logical_rows, ")"); } num_deletions_ = physical_rows - logical_rows; - } catch (const lance::LanceException& e) { - return arrow::Status::IOError("Failed to get physical row count for fragment ", fragment_id_, ": ", e.what()); } auto column_memory_sizes_result = @@ -396,9 +389,15 @@ arrow::Result> LanceTableReader::get_chunk(c // We add num_deletions_ to mitigate (1), but (2) is not addressed — if Lance // splits at page boundaries, chunk(0) will silently lose trailing rows in Release // builds (assert is a no-op). A robust fix would combine all chunks here. - ArrowArrayStream array_stream = - fragment_reader_->ReadRangesAsStream(start_idx, end_idx, end_idx - start_idx + num_deletions_); - ARROW_ASSIGN_OR_RAISE(auto chunkedarray, arrow::ImportChunkedArray(&array_stream)); + ARROW_ASSIGN_OR_RAISE(ArrowArrayStream array_stream, + fragment_reader_->ReadRangesAsStream(start_idx, end_idx, end_idx - start_idx + num_deletions_)); + // ImportChunkedArray drains the stream: mid-scan bridge errors surface here + // as raw marker-encoded strings and must be decoded (see bridge_error.h). + auto chunked_result = arrow::ImportChunkedArray(&array_stream); + if (!chunked_result.ok()) { + return milvus_storage::bridge::TranslateBridgeStatus("Failed to read lance chunk", chunked_result.status()); + } + auto chunkedarray = std::move(*chunked_result); assert(chunkedarray != nullptr && chunkedarray->num_chunks() == 1); return arrow::RecordBatch::FromStructArray(chunkedarray->chunk(0)); } @@ -437,10 +436,15 @@ arrow::Result>> LanceTableReader const auto& end_rg_info = row_group_infos_[rg_range.second]; // batch_size adds num_deletions_ for the same reason as get_chunk — see comment there. - ArrowArrayStream array_stream = + ARROW_ASSIGN_OR_RAISE( + ArrowArrayStream array_stream, fragment_reader_->ReadRangesAsStream(start_rg_info.start_offset, end_rg_info.end_offset, - end_rg_info.end_offset - start_rg_info.start_offset + num_deletions_); - ARROW_ASSIGN_OR_RAISE(auto chunkedarray, arrow::ImportChunkedArray(&array_stream)); + end_rg_info.end_offset - start_rg_info.start_offset + num_deletions_)); + auto chunked_result = arrow::ImportChunkedArray(&array_stream); + if (!chunked_result.ok()) { + return milvus_storage::bridge::TranslateBridgeStatus("Failed to read lance chunks", chunked_result.status()); + } + auto chunkedarray = std::move(*chunked_result); assert(chunkedarray != nullptr); // assign to rbs @@ -455,12 +459,16 @@ arrow::Result>> LanceTableReader arrow::Result> LanceTableReader::take(const std::vector& row_indices) { assert(fragment_reader_); - ArrowArrayStream array_stream = fragment_reader_->TakeAsStream(row_indices, row_indices.size()); - ARROW_ASSIGN_OR_RAISE(auto chunkedarray, arrow::ImportChunkedArray(&array_stream)); + ARROW_ASSIGN_OR_RAISE(ArrowArrayStream array_stream, fragment_reader_->TakeAsStream(row_indices, row_indices.size())); + auto chunked_result = arrow::ImportChunkedArray(&array_stream); + if (!chunked_result.ok()) { + return milvus_storage::bridge::TranslateBridgeStatus("Failed to take lance rows", chunked_result.status()); + } + auto chunkedarray = std::move(*chunked_result); // out of range if (chunkedarray->num_chunks() == 0) { - return arrow::Status::Invalid(fmt::format("out of row range [0, {}]", fragment_reader_->RowCount())); + return arrow::Status::Invalid(fmt::format("out of row range [0, {}]", fragment_reader_->RowCount().ValueOr(0))); } std::vector> rbs; @@ -477,9 +485,13 @@ arrow::Result> LanceTableReader::read_ assert(fragment_reader_); // Lance's read_range accepts logical indices directly. // batch_size adds num_deletions_ for the same reason as get_chunk — see comment there. - ArrowArrayStream array_stream = - fragment_reader_->ReadRangesAsStream(start_offset, end_offset, end_offset - start_offset + num_deletions_); - return arrow::ImportRecordBatchReader(&array_stream); + ARROW_ASSIGN_OR_RAISE( + ArrowArrayStream array_stream, + fragment_reader_->ReadRangesAsStream(start_offset, end_offset, end_offset - start_offset + num_deletions_)); + ARROW_ASSIGN_OR_RAISE(auto reader, arrow::ImportRecordBatchReader(&array_stream)); + // The stream stays live in the caller: wrap it so mid-scan bridge errors are + // decoded on every ReadNext instead of surfacing as raw marker strings. + return milvus_storage::bridge::WrapBridgeRecordBatchReader(std::move(reader), "Failed to read lance record batch"); } arrow::Result> LanceTableReader::clone_reader() { diff --git a/cpp/src/format/lance/lance_table_writer.cpp b/cpp/src/format/lance/lance_table_writer.cpp index d97e541a7..2808e239d 100644 --- a/cpp/src/format/lance/lance_table_writer.cpp +++ b/cpp/src/format/lance/lance_table_writer.cpp @@ -30,6 +30,9 @@ #include #include #include +#include + +#include #include "milvus-storage/format/lance/lance_common.h" @@ -123,23 +126,31 @@ arrow::Result LanceTableWriter::Close() { ARROW_ASSIGN_OR_RAISE(auto lance_uri, BuildLanceBaseUri(fs_config, base_path_)); if (!dataset_) { - try { - dataset_ = BlockingDataset::OpenUnique(lance_uri, storage_options); - origin_fids_ = dataset_->GetAllFragmentIds(); - dataset_->WriteArrowArrayStream(&array_stream); - } catch (std::exception& e) { - // dataset does not exist + auto open_result = BlockingDataset::OpenUnique(lance_uri, storage_options); + if (open_result.ok()) { + dataset_ = std::move(*open_result); + ARROW_ASSIGN_OR_RAISE(origin_fids_, dataset_->GetAllFragmentIds()); + ARROW_RETURN_NOT_OK(dataset_->WriteArrowArrayStream(&array_stream)); + } else if (arrow::internal::ErrnoFromStatus(open_result.status()) == ENOENT) { + // The dataset does not exist yet: create it. Only a classified + // not-found takes this path — auth failures, corruption, or transient + // IO errors now propagate instead of silently creating a fresh dataset + // (the previous catch(std::exception) treated every failure as + // "does not exist"). origin_fids_.clear(); - dataset_ = BlockingDataset::WriteDataset(lance_uri, &array_stream, storage_options, data_storage_format_); + ARROW_ASSIGN_OR_RAISE( + dataset_, BlockingDataset::WriteDataset(lance_uri, &array_stream, storage_options, data_storage_format_)); + } else { + return open_result.status(); } } else { - dataset_->WriteArrowArrayStream(&array_stream); + ARROW_RETURN_NOT_OK(dataset_->WriteArrowArrayStream(&array_stream)); } record_batches_.clear(); std::vector append_fids; std::vector current_fids; - current_fids = dataset_->GetAllFragmentIds(); + ARROW_ASSIGN_OR_RAISE(current_fids, dataset_->GetAllFragmentIds()); if (current_fids.size() < origin_fids_.size()) { return arrow::Status::Invalid( diff --git a/cpp/test/format/external_table_arn_test.cpp b/cpp/test/format/external_table_arn_test.cpp index db619bdcb..e7d02fdf3 100644 --- a/cpp/test/format/external_table_arn_test.cpp +++ b/cpp/test/format/external_table_arn_test.cpp @@ -188,8 +188,9 @@ class ExternalTableArnTest : public ::testing::TestWithParam { ARROW_RETURN_NOT_OK(ArrowFileSystemConfig::create_file_system_config(write_props_, write_config)); auto storage_options = iceberg::ToStorageOptions(write_config); - auto table_info = iceberg::CreateTestTable(table_uri, num_rows, false, {}, storage_options); - auto file_infos = iceberg::PlanFiles(table_info.metadata_location, table_info.snapshot_id, storage_options); + ARROW_ASSIGN_OR_RAISE(auto table_info, iceberg::CreateTestTable(table_uri, num_rows, false, {}, storage_options)); + ARROW_ASSIGN_OR_RAISE(auto file_infos, + iceberg::PlanFiles(table_info.metadata_location, table_info.snapshot_id, storage_options)); if (file_infos.empty()) { return arrow::Status::Invalid("PlanFiles returned no files"); } @@ -520,7 +521,8 @@ class ExternalTableGcpImpersonationTest : public ::testing::TestWithParam { ARROW_RETURN_NOT_OK(ArrowFileSystemConfig::create_file_system_config(properties_, fs_config)); auto storage_options = lance::ToStorageOptions(fs_config); - auto dataset = lance::BlockingDataset::Open(lance_uri, storage_options); + ARROW_ASSIGN_OR_RAISE(auto dataset, lance::BlockingDataset::Open(lance_uri, storage_options)); // Build predicate like "id in (3, 10, 25)" std::string predicate = "id in ("; @@ -287,7 +287,7 @@ class ExternalTableTest : public ::testing::TestWithParam { predicate += std::to_string(deleted_ids[i]); } predicate += ")"; - dataset->DeleteRows(predicate); + ARROW_RETURN_NOT_OK(dataset->DeleteRows(predicate)); return WriteResult{std::move(result.cgfile), result.schema, num_rows}; } @@ -301,8 +301,9 @@ class ExternalTableTest : public ::testing::TestWithParam { auto table_uri = MakeTableUri(bucket, path); auto storage_options = iceberg::ToStorageOptions(fs_config_); - auto table_info = iceberg::CreateTestTable(table_uri, num_rows, false, {}, storage_options); - auto file_infos = iceberg::PlanFiles(table_info.metadata_location, table_info.snapshot_id, storage_options); + ARROW_ASSIGN_OR_RAISE(auto table_info, iceberg::CreateTestTable(table_uri, num_rows, false, {}, storage_options)); + ARROW_ASSIGN_OR_RAISE(auto file_infos, + iceberg::PlanFiles(table_info.metadata_location, table_info.snapshot_id, storage_options)); if (file_infos.empty()) { return arrow::Status::Invalid("PlanFiles returned no files"); } @@ -321,8 +322,10 @@ class ExternalTableTest : public ::testing::TestWithParam { auto table_uri = MakeTableUri(bucket, path); auto storage_options = iceberg::ToStorageOptions(fs_config_); - auto table_info = iceberg::CreateTestTable(table_uri, num_rows, true, deleted_ids, storage_options); - auto file_infos = iceberg::PlanFiles(table_info.metadata_location, table_info.snapshot_id, storage_options); + ARROW_ASSIGN_OR_RAISE(auto table_info, + iceberg::CreateTestTable(table_uri, num_rows, true, deleted_ids, storage_options)); + ARROW_ASSIGN_OR_RAISE(auto file_infos, + iceberg::PlanFiles(table_info.metadata_location, table_info.snapshot_id, storage_options)); if (file_infos.empty()) { return arrow::Status::Invalid("PlanFiles returned no files"); } @@ -519,8 +522,10 @@ class ExternalSplitColumnGroupTest : public ::testing::TestWithParam file_infos; try { auto storage_options = iceberg::ToStorageOptions(fs_config_); - table_info = iceberg::CreateTestTable(table_uri, kExpectedRows, false, {}, storage_options); - file_infos = iceberg::PlanFiles(table_info.metadata_location, table_info.snapshot_id, storage_options); + table_info = iceberg::CreateTestTable(table_uri, kExpectedRows, false, {}, storage_options).ValueOrDie(); + file_infos = + iceberg::PlanFiles(table_info.metadata_location, table_info.snapshot_id, storage_options).ValueOrDie(); } catch (const std::exception& e) { return arrow::Status::IOError("Failed to create Iceberg stress table: ", e.what()); } diff --git a/cpp/test/format/iceberg/iceberg_bridge_test.cpp b/cpp/test/format/iceberg/iceberg_bridge_test.cpp index 6233884a6..b6dee8463 100644 --- a/cpp/test/format/iceberg/iceberg_bridge_test.cpp +++ b/cpp/test/format/iceberg/iceberg_bridge_test.cpp @@ -22,35 +22,31 @@ namespace milvus_storage::iceberg { class IcebergBridgeTest : public ::testing::Test {}; -// PlanFiles should throw IcebergException for a nonexistent local metadata file +// PlanFiles should return an error status for a nonexistent local metadata file TEST_F(IcebergBridgeTest, PlanFilesNonexistentLocalMetadata) { std::unordered_map opts; - EXPECT_THROW(PlanFiles("/nonexistent/path/v1.metadata.json", 1, opts), IcebergException); + EXPECT_FALSE(PlanFiles("/nonexistent/path/v1.metadata.json", 1, opts).ok()); } -// PlanFiles should throw IcebergException for an empty metadata location +// PlanFiles should return an error status for an empty metadata location TEST_F(IcebergBridgeTest, PlanFilesEmptyMetadataLocation) { std::unordered_map opts; - EXPECT_THROW(PlanFiles("", 1, opts), IcebergException); + EXPECT_FALSE(PlanFiles("", 1, opts).ok()); } -// PlanFiles should throw IcebergException with an invalid snapshot id +// PlanFiles should return an error status with an invalid snapshot id // even if the metadata file does not exist TEST_F(IcebergBridgeTest, PlanFilesInvalidSnapshotId) { std::unordered_map opts; - EXPECT_THROW(PlanFiles("file:///nonexistent/metadata.json", -999, opts), IcebergException); + EXPECT_FALSE(PlanFiles("file:///nonexistent/metadata.json", -999, opts).ok()); } -// Verify IcebergException carries a descriptive message -TEST_F(IcebergBridgeTest, ExceptionMessageIsDescriptive) { +// Verify the returned error status carries a descriptive message +TEST_F(IcebergBridgeTest, ErrorStatusMessageIsDescriptive) { std::unordered_map opts; - try { - PlanFiles("/nonexistent/v1.metadata.json", 1, opts); - FAIL() << "Expected IcebergException"; - } catch (const IcebergException& e) { - std::string msg = e.what(); - EXPECT_FALSE(msg.empty()) << "Exception message should not be empty"; - } + auto result = PlanFiles("/nonexistent/v1.metadata.json", 1, opts); + ASSERT_FALSE(result.ok()); + EXPECT_FALSE(result.status().message().empty()) << "Error message should not be empty"; } // IcebergFileInfo default construction diff --git a/cpp/test/format/iceberg/iceberg_integration_test.cpp b/cpp/test/format/iceberg/iceberg_integration_test.cpp index 71137b2e0..20997de3b 100644 --- a/cpp/test/format/iceberg/iceberg_integration_test.cpp +++ b/cpp/test/format/iceberg/iceberg_integration_test.cpp @@ -80,11 +80,11 @@ TEST_F(IcebergIntegrationTest, ExploreAndReadBasic) { const uint64_t num_rows = 50; // 1. Create a standard Iceberg table via Rust bridge - auto table_info = CreateTestTable(abs_table_dir_, num_rows, false, {}); + auto table_info = CreateTestTable(abs_table_dir_, num_rows, false, {}).ValueOrDie(); // 2. Explore: plan files from the Iceberg metadata std::unordered_map storage_options; - auto file_infos = PlanFiles(table_info.metadata_location, table_info.snapshot_id, storage_options); + auto file_infos = PlanFiles(table_info.metadata_location, table_info.snapshot_id, storage_options).ValueOrDie(); ASSERT_EQ(file_infos.size(), 1); ASSERT_EQ(file_infos[0].record_count, num_rows); @@ -139,11 +139,11 @@ TEST_F(IcebergIntegrationTest, ExploreAndReadWithPositionalDeletes) { std::vector deleted_positions = {3, 7, 15}; // 1. Create Iceberg table with positional deletes - auto table_info = CreateTestTable(abs_table_dir_, num_rows, true, deleted_positions); + auto table_info = CreateTestTable(abs_table_dir_, num_rows, true, deleted_positions).ValueOrDie(); // 2. Explore std::unordered_map storage_options; - auto file_infos = PlanFiles(table_info.metadata_location, table_info.snapshot_id, storage_options); + auto file_infos = PlanFiles(table_info.metadata_location, table_info.snapshot_id, storage_options).ValueOrDie(); ASSERT_EQ(file_infos.size(), 1); ASSERT_EQ(file_infos[0].record_count, num_rows); @@ -201,10 +201,10 @@ TEST_F(IcebergIntegrationTest, TakeWithPositionalDeletes) { const uint64_t num_rows = 30; std::vector deleted_positions = {5, 10, 20}; - auto table_info = CreateTestTable(abs_table_dir_, num_rows, true, deleted_positions); + auto table_info = CreateTestTable(abs_table_dir_, num_rows, true, deleted_positions).ValueOrDie(); std::unordered_map storage_options; - auto file_infos = PlanFiles(table_info.metadata_location, table_info.snapshot_id, storage_options); + auto file_infos = PlanFiles(table_info.metadata_location, table_info.snapshot_id, storage_options).ValueOrDie(); ASSERT_EQ(file_infos.size(), 1); auto cg_file = MakeCgFile(file_infos[0]); @@ -238,10 +238,10 @@ TEST_F(IcebergIntegrationTest, TakeWithPositionalDeletes) { TEST_F(IcebergIntegrationTest, ColumnProjection) { const uint64_t num_rows = 10; - auto table_info = CreateTestTable(abs_table_dir_, num_rows, false, {}); + auto table_info = CreateTestTable(abs_table_dir_, num_rows, false, {}).ValueOrDie(); std::unordered_map storage_options; - auto file_infos = PlanFiles(table_info.metadata_location, table_info.snapshot_id, storage_options); + auto file_infos = PlanFiles(table_info.metadata_location, table_info.snapshot_id, storage_options).ValueOrDie(); ASSERT_EQ(file_infos.size(), 1); auto cg_file = MakeCgFile(file_infos[0]); @@ -266,10 +266,10 @@ TEST_F(IcebergIntegrationTest, CloneReaderSharesDeletes) { const uint64_t num_rows = 10; std::vector deleted_positions = {2, 8}; - auto table_info = CreateTestTable(abs_table_dir_, num_rows, true, deleted_positions); + auto table_info = CreateTestTable(abs_table_dir_, num_rows, true, deleted_positions).ValueOrDie(); std::unordered_map storage_options; - auto file_infos = PlanFiles(table_info.metadata_location, table_info.snapshot_id, storage_options); + auto file_infos = PlanFiles(table_info.metadata_location, table_info.snapshot_id, storage_options).ValueOrDie(); auto cg_file = MakeCgFile(file_infos[0]); diff --git a/cpp/test/format/lance/lance_bridge_error_test.cpp b/cpp/test/format/lance/lance_bridge_error_test.cpp new file mode 100644 index 000000000..e71b4c256 --- /dev/null +++ b/cpp/test/format/lance/lance_bridge_error_test.cpp @@ -0,0 +1,112 @@ +// Copyright 2025 Zilliz +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +// Regression tests for the classified error channel of the Rust bridges: +// the Rust side embeds a marker code into the (string-only) cxx error, the +// shared decoder rebuilds a structured arrow::Status, and the segcore mapping +// turns it into the right ErrorCode. These tests pin both the decoder table +// and the end-to-end not-found path through a real lance open. + +#include +#include + +#include +#include +#include + +#include "bridge_error.h" +#include "lance_bridge.h" +#include "milvus-storage/common/extend_status.h" +#include "milvus-storage/ffi_internal/ffi_error_code.h" + +namespace milvus_storage::bridge { +namespace { + +constexpr const char* kMarker = "__LOON_VORTEX_FFI_ERRCODE__="; + +TEST(BridgeErrorTest, NotFoundCodeBecomesEnoentDetail) { + auto status = MakeBridgeErrorStatus(std::string(kMarker) + "12; dataset was not found"); + ASSERT_TRUE(status.IsIOError()); + EXPECT_EQ(arrow::internal::ErrnoFromStatus(status), ENOENT); + // Marker must be stripped from the user-visible message. + EXPECT_EQ(status.message().find("__LOON_"), std::string::npos); + // End of the chain: fine-grained ObjectNotExist, never a transient code. + EXPECT_EQ(ToSegcoreError(status).get_error_code(), milvus::ObjectNotExist); +} + +TEST(BridgeErrorTest, TransientCodeBecomesRetryableExtendDetail) { + auto status = MakeBridgeErrorStatus(std::string(kMarker) + "109; too much write contention"); + ASSERT_TRUE(status.IsIOError()); + auto detail = ExtendStatusDetail::UnwrapStatus(status); + ASSERT_NE(detail, nullptr); + EXPECT_EQ(detail->code(), ExtendStatusCode::StorageTransientThrottling); + EXPECT_TRUE(detail->retryable()); + EXPECT_EQ(ToSegcoreError(status).get_error_code(), milvus::StorageTransientError); +} + +TEST(BridgeErrorTest, BridgePrivateCodesMapToArrowStatusCodes) { + auto corrupt = MakeBridgeErrorStatus(std::string(kMarker) + "1001; corrupt file"); + EXPECT_TRUE(corrupt.IsInvalid()); + EXPECT_EQ(ToSegcoreError(corrupt).get_error_code(), milvus::DataFormatBroken); + + auto not_supported = MakeBridgeErrorStatus(std::string(kMarker) + "1002; unsupported feature"); + EXPECT_TRUE(not_supported.IsNotImplemented()); +} + +TEST(BridgeErrorTest, UnmarkedMessageStaysConservativeIOError) { + auto status = MakeBridgeErrorStatus("some opaque failure"); + ASSERT_TRUE(status.IsIOError()); + EXPECT_EQ(ExtendStatusDetail::UnwrapStatus(status), nullptr); + // Untagged -> conservative non-retriable StorageError, never invented + // retriability. + EXPECT_EQ(ToSegcoreError(status).get_error_code(), milvus::StorageError); +} + +TEST(BridgeErrorTest, UnknownMarkerCodeFallsBackToIOError) { + auto status = MakeBridgeErrorStatus(std::string(kMarker) + "424242; from a future version"); + ASSERT_TRUE(status.IsIOError()); + EXPECT_EQ(ExtendStatusDetail::UnwrapStatus(status), nullptr); + EXPECT_EQ(ToSegcoreError(status).get_error_code(), milvus::StorageError); +} + +TEST(BridgeErrorTest, TranslatePreservesClassificationAndAddsContext) { + auto tagged = MakeBridgeErrorStatus(std::string(kMarker) + "109; throttled"); + auto translated = TranslateBridgeStatus("reading chunk", tagged); + auto detail = ExtendStatusDetail::UnwrapStatus(translated); + ASSERT_NE(detail, nullptr); + EXPECT_EQ(detail->code(), ExtendStatusCode::StorageTransientThrottling); + EXPECT_NE(translated.message().find("reading chunk"), std::string::npos); + + // A status whose *message* still carries the marker (arrow FFI stream + // stringification, the mid-scan case) is decoded too. + auto raw = arrow::Status::IOError(std::string(kMarker) + "12; object vanished mid-scan"); + auto decoded = TranslateBridgeStatus("stream", raw); + EXPECT_EQ(arrow::internal::ErrnoFromStatus(decoded), ENOENT); + EXPECT_EQ(decoded.message().find("__LOON_"), std::string::npos); +} + +// End-to-end: a real lance open against a nonexistent local dataset must come +// back as a classified not-found (ENOENT detail -> ObjectNotExist), not as an +// exception and not as an opaque IOError. +TEST(LanceBridgeErrorTest, OpenNonexistentDatasetClassifiesNotFound) { + auto result = milvus_storage::lance::BlockingDataset::Open("/nonexistent-milvus-storage-test/lance-dataset"); + ASSERT_FALSE(result.ok()); + const auto& status = result.status(); + EXPECT_EQ(arrow::internal::ErrnoFromStatus(status), ENOENT) << status.ToString(); + EXPECT_EQ(status.message().find("__LOON_"), std::string::npos) << status.ToString(); + EXPECT_EQ(milvus_storage::ToSegcoreError(status).get_error_code(), milvus::ObjectNotExist) << status.ToString(); +} + +} // namespace +} // namespace milvus_storage::bridge diff --git a/cpp/test/format/lance/lance_table_test.cpp b/cpp/test/format/lance/lance_table_test.cpp index 93e79aef0..c86ca9cbd 100644 --- a/cpp/test/format/lance/lance_table_test.cpp +++ b/cpp/test/format/lance/lance_table_test.cpp @@ -303,8 +303,8 @@ TEST_F(LanceBasicTest, TestBasic) { } auto verify_reader = [&]() { - auto read_dataset = BlockingDataset::Open(lance_uri, storage_options); - const std::vector fragment_ids = read_dataset->GetAllFragmentIds(); + auto read_dataset = BlockingDataset::Open(lance_uri, storage_options).ValueOrDie(); + const std::vector fragment_ids = read_dataset->GetAllFragmentIds().ValueOrDie(); uint64_t total_rows = 0; for (const auto& fragment_id : fragment_ids) { @@ -367,7 +367,7 @@ TEST_F(LanceBasicTest, TestReaderHandlesFragmentMissingNullableDatasetColumn) { ASSERT_AND_ASSIGN(auto parsed_uri, ParseLanceUri(appended_file.path)); ArrowFileSystemConfig fs_config; ASSERT_STATUS_OK(ArrowFileSystemConfig::create_file_system_config(properties_, fs_config)); - auto dataset = BlockingDataset::Open(ToStandardLanceUri(parsed_uri.first), ToStorageOptions(fs_config)); + auto dataset = BlockingDataset::Open(ToStandardLanceUri(parsed_uri.first), ToStorageOptions(fs_config)).ValueOrDie(); LanceTableReader reader(dataset, parsed_uri.second, nullptr, properties_); ASSERT_STATUS_OK(reader.open()); @@ -403,9 +403,9 @@ TEST_F(LanceBasicTest, TestRead) { ASSERT_AND_ASSIGN(auto cgfile, writer.Close()); ASSERT_EQ(cgfile.end_index, large_batch->num_rows()); - auto read_dataset = BlockingDataset::Open(lance_uri, storage_options); + auto read_dataset = BlockingDataset::Open(lance_uri, storage_options).ValueOrDie(); - const std::vector fragment_ids = read_dataset->GetAllFragmentIds(); + const std::vector fragment_ids = read_dataset->GetAllFragmentIds().ValueOrDie(); // The splitting conditions(`WriteParams`) in lance are very strict. // So the default setting will only generate one fragment. ASSERT_EQ(fragment_ids.size(), 1); @@ -507,10 +507,10 @@ TEST_F(LanceBasicTest, EstimatedMemoryAccountsForDeletions) { ASSERT_STATUS_OK(writer.Write(batch)); ASSERT_AND_ASSIGN(auto cgfile, writer.Close()); ASSERT_EQ(cgfile.end_index, kRows); - auto dataset = BlockingDataset::Open(lance_uri, storage_options); - dataset->DeleteRows("id < 2000"); + auto dataset = BlockingDataset::Open(lance_uri, storage_options).ValueOrDie(); + ASSERT_STATUS_OK(dataset->DeleteRows("id < 2000")); - auto fragment_ids = dataset->GetAllFragmentIds(); + auto fragment_ids = dataset->GetAllFragmentIds().ValueOrDie(); ASSERT_EQ(fragment_ids.size(), 1); LanceTableReader reader(dataset, fragment_ids[0], id_schema, properties_); ASSERT_STATUS_OK(reader.open()); @@ -622,8 +622,8 @@ TEST_F(LanceBasicTest, LegacyFormatReadsWhenMemoryEstimateIsUnavailable) { ASSERT_AND_ASSIGN(auto cgfile, writer.Close()); ASSERT_EQ(cgfile.end_index, kRows); - auto dataset = BlockingDataset::Open(lance_uri, storage_options); - auto fragment_ids = dataset->GetAllFragmentIds(); + auto dataset = BlockingDataset::Open(lance_uri, storage_options).ValueOrDie(); + auto fragment_ids = dataset->GetAllFragmentIds().ValueOrDie(); ASSERT_EQ(fragment_ids.size(), 1); auto estimate_result = dataset->EstimateFragmentColumnMemory(fragment_ids[0]); diff --git a/cpp/test/tools/loon_test.cpp b/cpp/test/tools/loon_test.cpp index 31314f0f6..3304b85a4 100644 --- a/cpp/test/tools/loon_test.cpp +++ b/cpp/test/tools/loon_test.cpp @@ -79,11 +79,11 @@ TEST_F(LoonTest, CreateAndReadIceberg) { const uint64_t num_rows = 30; // 1. Create Iceberg test table - auto table_info = CreateTestTable(table_dir_, num_rows, false, {}); + auto table_info = CreateTestTable(table_dir_, num_rows, false, {}).ValueOrDie(); // 2. Explore via PlanFiles std::unordered_map storage_options; - auto file_infos = PlanFiles(table_info.metadata_location, table_info.snapshot_id, storage_options); + auto file_infos = PlanFiles(table_info.metadata_location, table_info.snapshot_id, storage_options).ValueOrDie(); ASSERT_EQ(file_infos.size(), 1); // 3. Build ColumnGroup and commit manifest via Transaction @@ -145,10 +145,10 @@ TEST_F(LoonTest, CreateAndTakeWithDeletes) { const uint64_t num_rows = 20; std::vector deleted_positions = {3, 7, 15}; - auto table_info = CreateTestTable(table_dir_, num_rows, true, deleted_positions); + auto table_info = CreateTestTable(table_dir_, num_rows, true, deleted_positions).ValueOrDie(); std::unordered_map storage_options; - auto file_infos = PlanFiles(table_info.metadata_location, table_info.snapshot_id, storage_options); + auto file_infos = PlanFiles(table_info.metadata_location, table_info.snapshot_id, storage_options).ValueOrDie(); ASSERT_EQ(file_infos.size(), 1); ASSERT_FALSE(file_infos[0].delete_metadata_json.empty()); @@ -199,10 +199,10 @@ TEST_F(LoonTest, SequentialReadFiltersDeletes) { const uint64_t num_rows = 15; std::vector deleted_positions = {0, 5, 14}; - auto table_info = CreateTestTable(table_dir_, num_rows, true, deleted_positions); + auto table_info = CreateTestTable(table_dir_, num_rows, true, deleted_positions).ValueOrDie(); std::unordered_map storage_options; - auto file_infos = PlanFiles(table_info.metadata_location, table_info.snapshot_id, storage_options); + auto file_infos = PlanFiles(table_info.metadata_location, table_info.snapshot_id, storage_options).ValueOrDie(); std::vector files; { @@ -261,10 +261,10 @@ TEST_F(LoonTest, ManifestPreservesDeleteMetadata) { const uint64_t num_rows = 10; std::vector deleted_positions = {2, 8}; - auto table_info = CreateTestTable(table_dir_, num_rows, true, deleted_positions); + auto table_info = CreateTestTable(table_dir_, num_rows, true, deleted_positions).ValueOrDie(); std::unordered_map storage_options; - auto file_infos = PlanFiles(table_info.metadata_location, table_info.snapshot_id, storage_options); + auto file_infos = PlanFiles(table_info.metadata_location, table_info.snapshot_id, storage_options).ValueOrDie(); // Commit manifest with delete metadata std::vector files; diff --git a/cpp/tools/loon.cpp b/cpp/tools/loon.cpp index 8309bb7e1..f4dcdd9a0 100644 --- a/cpp/tools/loon.cpp +++ b/cpp/tools/loon.cpp @@ -151,8 +151,7 @@ static int DoDemoTable(int argc, char** argv) { std::cerr << std::endl; std::cerr << "Types: iceberg" << std::endl; std::cerr << std::endl; - std::cerr << "Creates a demo table with schema (id int64, name string," - << " value float64)." << std::endl; + std::cerr << "Creates a demo table with schema (id int64, name string," << " value float64)." << std::endl; std::cerr << R"(Data: id=0..N-1, name="row_0".."row_{N-1}", value=id*1.5)" << std::endl; std::cerr << std::endl; std::cerr << "For cloud storage, pass extfs.* properties via --prop." << std::endl; @@ -192,7 +191,13 @@ static int DoDemoTable(int argc, char** argv) { } bool with_deletes = !deletes.empty(); - auto info = milvus_storage::iceberg::CreateTestTable(table_path, rows, with_deletes, deletes, storage_options); + auto info_result = + milvus_storage::iceberg::CreateTestTable(table_path, rows, with_deletes, deletes, storage_options); + if (!info_result.ok()) { + std::cerr << "Failed to create iceberg table: " << info_result.status().ToString() << std::endl; + return 1; + } + auto info = std::move(*info_result); std::cout << "Created iceberg table:" << std::endl; std::cout << " path: " << table_path << std::endl; @@ -283,12 +288,12 @@ static arrow::Result> ExploreLance(const std::strin ARROW_ASSIGN_OR_RAISE(auto lance_base_uri, milvus_storage::lance::BuildLanceBaseUri(fs_config, resolved_dir)); auto storage_options = milvus_storage::lance::ToStorageOptions(fs_config); - auto dataset = milvus_storage::lance::BlockingDataset::Open(lance_base_uri, storage_options); - auto fragment_ids = dataset->GetAllFragmentIds(); + ARROW_ASSIGN_OR_RAISE(auto dataset, milvus_storage::lance::BlockingDataset::Open(lance_base_uri, storage_options)); + ARROW_ASSIGN_OR_RAISE(auto fragment_ids, dataset->GetAllFragmentIds()); std::vector files; for (auto frag_id : fragment_ids) { - auto row_count = dataset->GetFragmentRowCount(frag_id); + ARROW_ASSIGN_OR_RAISE(auto row_count, dataset->GetFragmentRowCount(frag_id)); files.emplace_back( ColumnGroupFile{milvus_storage::lance::MakeLanceUri( milvus_storage::lance::ToMilvusLanceUri(lance_base_uri, fs_config.address), frag_id), @@ -311,7 +316,7 @@ static arrow::Result> ExploreIceberg(const std::str ARROW_ASSIGN_OR_RAISE(auto parsed_uri, StorageUri::Parse(source)); ARROW_ASSIGN_OR_RAISE(auto iceberg_uri, StorageUri::Make(parsed_uri, false)); - auto file_infos = milvus_storage::iceberg::PlanFiles(iceberg_uri, snapshot_id, storage_options); + ARROW_ASSIGN_OR_RAISE(auto file_infos, milvus_storage::iceberg::PlanFiles(iceberg_uri, snapshot_id, storage_options)); std::vector files; files.reserve(file_infos.size()); @@ -356,8 +361,7 @@ static int DoCreate(int argc, char** argv) { if (format.empty() || source.empty() || target.empty() || columns.empty()) { std::cerr << "Usage: loon create --format --source " - << "--target --columns col1,col2,... " - << "[--prop key=value ...]" << std::endl; + << "--target --columns col1,col2,... " << "[--prop key=value ...]" << std::endl; std::cerr << std::endl; std::cerr << "Formats: parquet, vortex, lance-table, iceberg-table" << std::endl; std::cerr << std::endl; @@ -563,8 +567,7 @@ static int DoDescribe(int argc, char** argv) { static int DoRead(int argc, char** argv) { if (argc < 1) { std::cerr << "Usage: loon read --columns col1,col2,..." - << " [--take pos1,pos2,...] [--predicate \"expr\"]" - << " [--verbose] [--prop key=value ...]" << std::endl; + << " [--take pos1,pos2,...] [--predicate \"expr\"]" << " [--verbose] [--prop key=value ...]" << std::endl; return 1; } std::string manifest_path = argv[0]; @@ -635,8 +638,8 @@ static int DoRead(int argc, char** argv) { for (size_t fi = 0; fi < cg->files.size(); ++fi) { auto& f = cg->files[fi]; std::cout << " file[" << fi << "] path=" << f.path << " range=[" << f.start_index << "," << f.end_index - << ")" - << " has_metadata=" << (f.properties.count(kPropertyMetadata) > 0 ? "true" : "false") << std::endl; + << ")" << " has_metadata=" << (f.properties.count(kPropertyMetadata) > 0 ? "true" : "false") + << std::endl; auto meta_it = f.properties.find(kPropertyMetadata); if (meta_it != f.properties.end()) { std::cout << " metadata: " << meta_it->second << std::endl; From 9f5ce26fb2fb7e67ee6f92608b8f1e4a2771cf54 Mon Sep 17 00:00:00 2001 From: "zhenshan.cao" Date: Mon, 27 Jul 2026 17:32:30 -0700 Subject: [PATCH 2/9] address review: classification fidelity and writer stream cleanup Adopts the adversarial-review findings on #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) Signed-off-by: zhenshan.cao --- .../format/bridge/rust/src/bridge_error.cpp | 8 +- .../format/bridge/rust/src/bridge_error.rs | 114 ++++++++++++++++-- cpp/src/format/lance/lance_table_writer.cpp | 13 ++ .../format/lance/lance_bridge_error_test.cpp | 16 +++ cpp/test/format/lance/lance_table_test.cpp | 47 ++++++++ 5 files changed, 189 insertions(+), 9 deletions(-) diff --git a/cpp/src/format/bridge/rust/src/bridge_error.cpp b/cpp/src/format/bridge/rust/src/bridge_error.cpp index 8f646af8c..edb677501 100644 --- a/cpp/src/format/bridge/rust/src/bridge_error.cpp +++ b/cpp/src/format/bridge/rust/src/bridge_error.cpp @@ -138,8 +138,12 @@ arrow::Status TranslateBridgeStatus(std::string_view context, const arrow::Statu if (status.ok()) { return status; } - if (ExtendStatusDetail::UnwrapStatus(status) || arrow::internal::ErrnoFromStatus(status) == ENOENT) { - // Already structured — nothing to decode. + if (ExtendStatusDetail::UnwrapStatus(status) || arrow::internal::ErrnoFromStatus(status) == ENOENT || + !status.IsIOError()) { + // Already structured, or not a bridge-encoded carrier at all: bridge + // errors only ever surface as IOError strings (arrow FFI stringification), + // so re-decoding e.g. an Invalid or OutOfMemory from arrow itself would + // DOWNGRADE its StatusCode to IOError. Pass those through untouched. return WithBridgeContext(context, status); } return WithBridgeContext(context, MakeBridgeErrorStatus(status.message())); diff --git a/cpp/src/format/bridge/rust/src/bridge_error.rs b/cpp/src/format/bridge/rust/src/bridge_error.rs index ef4290940..823d2ef35 100644 --- a/cpp/src/format/bridge/rust/src/bridge_error.rs +++ b/cpp/src/format/bridge/rust/src/bridge_error.rs @@ -46,7 +46,9 @@ pub const LOON_FILE_NOT_FOUND: i32 = 12; /// Mirror of the ExtendStatusCode transient tags (`ffi_error_code.h` 101-112). pub const LOON_AWS_ERROR_PRECONDITION_FAILED: i32 = 103; pub const LOON_AWS_ERROR_ACCESS_DENIED: i32 = 105; +pub const LOON_TRANSIENT_TIMEOUT: i32 = 108; pub const LOON_TRANSIENT_THROTTLING: i32 = 109; +pub const LOON_TRANSIENT_SERVICE: i32 = 110; /// Bridge-private codes (>= 1000): decoded by cpp `bridge_error.cpp` into an /// arrow StatusCode, never forwarded as an FFI error code. @@ -89,12 +91,19 @@ pub fn classify_lance_error(e: &LanceError) -> Option { | LanceError::DatasetNotFound { .. } | LanceError::IndexNotFound { .. } | LanceError::RefNotFound { .. } - | LanceError::VersionNotFound { .. } - | LanceError::FieldNotFound { .. } => Some(LOON_FILE_NOT_FOUND), - // Permanent data problems: retrying re-reads the same bytes. - LanceError::CorruptFile { .. } + | LanceError::VersionNotFound { .. } => Some(LOON_FILE_NOT_FOUND), + // Field/schema errors are caller/schema-evolution conditions, not + // corruption -- and they must NOT look like a missing dataset: the + // ENOENT classification drives create-if-missing in the lance writer, + // so tagging FieldNotFound as not-found could turn a projection typo + // into a dataset-creation attempt. Producer sites are mixed + // (library-assembled schemas vs user projections), so they stay + // untagged -> conservative non-retriable. + LanceError::FieldNotFound { .. } | LanceError::SchemaMismatch { .. } - | LanceError::Schema { .. } => Some(BRIDGE_ERRCODE_DATA_CORRUPT), + | LanceError::Schema { .. } => None, + // Permanent data problems: retrying re-reads the same bytes. + LanceError::CorruptFile { .. } => Some(BRIDGE_ERRCODE_DATA_CORRUPT), LanceError::NotSupported { .. } => Some(BRIDGE_ERRCODE_NOT_SUPPORTED), // Lance itself declares these retryable: the failed attempt is spent, // but a fresh attempt (new commit round) can succeed. This is the @@ -117,8 +126,18 @@ pub fn classify_lance_error(e: &LanceError) -> Option { object_store::Error::NotSupported { .. } | object_store::Error::NotImplemented { .. }, ) => Some(BRIDGE_ERRCODE_NOT_SUPPORTED), - // Generic and friends: object_store has already spent its own - // retry budget; no positive transient/permanent signal survives, + // Generic carries the post-retry HTTP failure. The typed carrier + // (client::retry::RetryError, which has .status()) is pub(crate) + // in object_store and cannot be downcast from here, so the status + // code is recovered from the stable Display pattern of + // RequestError::Status ("non-2xx status code: NNN"). Fail-safe by + // construction: if object_store ever rewords it, this returns + // None and the error lands in the conservative non-retriable + // bucket -- it can never mis-tag a permanent error as transient. + Some(object_store::Error::Generic { source, .. }) => { + classify_http_status_in_message(&source.to_string()) + } + // Anything else: no positive transient/permanent signal survives, // so stay untagged (conservative). _ => None, }, @@ -130,6 +149,24 @@ pub fn classify_lance_error(e: &LanceError) -> Option { } } +/// Recover the HTTP status from object_store's post-retry error message +/// ("Server returned non-2xx status code: NNN: ..."). Only well-known +/// transient statuses are tagged; anything else stays untagged. +fn classify_http_status_in_message(msg: &str) -> Option { + const PATTERN: &str = "non-2xx status code: "; + let idx = msg.find(PATTERN)?; + let digits: String = msg[idx + PATTERN.len()..] + .chars() + .take_while(|c| c.is_ascii_digit()) + .collect(); + match digits.parse::().ok()? { + 408 => Some(LOON_TRANSIENT_TIMEOUT), + 429 => Some(LOON_TRANSIENT_THROTTLING), + 500 | 502 | 503 | 504 => Some(LOON_TRANSIENT_SERVICE), + _ => None, + } +} + impl From for BridgeError { fn from(e: LanceError) -> Self { BridgeError { @@ -147,3 +184,66 @@ impl From for BridgeError { } } } + +#[cfg(test)] +mod tests { + use super::*; + + // Compile-time version pin: LanceError's From impl only accepts the + // object_store version lance itself depends on. If this crate's + // object_store ever diverges from lance's, this test stops COMPILING, + // surfacing the downcast coupling instead of letting classify_lance_error + // silently fail at runtime. + #[test] + fn object_store_version_matches_lance() { + let e = LanceError::from(object_store::Error::NotFound { + path: "p".to_string(), + source: "gone".into(), + }); + assert_eq!(classify_lance_error(&e), Some(LOON_FILE_NOT_FOUND)); + } + + #[test] + fn generic_throttle_status_is_tagged_transient() { + let e = LanceError::from(object_store::Error::Generic { + store: "S3", + source: "Error performing GET https://x in 30s, after 10 retries - Server returned non-2xx status code: 429: slow down" + .into(), + }); + assert_eq!(classify_lance_error(&e), Some(LOON_TRANSIENT_THROTTLING)); + + let e503 = LanceError::from(object_store::Error::Generic { + store: "S3", + source: "Server returned non-2xx status code: 503: unavailable".into(), + }); + assert_eq!(classify_lance_error(&e503), Some(LOON_TRANSIENT_SERVICE)); + } + + #[test] + fn generic_without_status_stays_untagged() { + let e = LanceError::from(object_store::Error::Generic { + store: "S3", + source: "connection reset by peer".into(), + }); + assert_eq!(classify_lance_error(&e), None); + // 4xx that is NOT a known transient must never be tagged retryable. + let e404ish = LanceError::from(object_store::Error::Generic { + store: "S3", + source: "Server returned non-2xx status code: 400: bad request".into(), + }); + assert_eq!(classify_lance_error(&e404ish), None); + } + + #[test] + fn field_and_schema_errors_are_not_enoent() { + // FieldNotFound must never classify as file-not-found: ENOENT drives + // create-if-missing in the lance writer. + let e = LanceError::FieldNotFound { + source: lance_core::error::FieldNotFoundError { + field_name: "f".to_string(), + candidates: vec![], + }, + }; + assert_eq!(classify_lance_error(&e), None); + } +} diff --git a/cpp/src/format/lance/lance_table_writer.cpp b/cpp/src/format/lance/lance_table_writer.cpp index 2808e239d..4d11f7f41 100644 --- a/cpp/src/format/lance/lance_table_writer.cpp +++ b/cpp/src/format/lance/lance_table_writer.cpp @@ -16,6 +16,7 @@ #include "milvus-storage/format/lance/lance_table_writer.h" +#include #include #include #include @@ -116,6 +117,18 @@ arrow::Result LanceTableWriter::Close() { auto batch_iterator = std::make_shared(schema_, record_batches_); ARROW_RETURN_NOT_OK(ExportRecordBatchReader(batch_iterator, &array_stream)); + // The exported stream owns the batch iterator (and thereby every buffered + // RecordBatch). The Rust write entry points take ownership immediately + // (ptr::replace with an empty stream, so `release` becomes null and this + // guard is a no-op) -- but the error returns BEFORE the stream is handed to + // Rust (open failure, fragment-id listing failure) would otherwise leak the + // whole write payload. + auto stream_guard = [](struct ArrowArrayStream* s) { + if (s->release != nullptr) { + s->release(s); + } + }; + std::unique_ptr release_on_exit(&array_stream, stream_guard); // Get storage options from properties for cloud storage support ArrowFileSystemConfig fs_config; diff --git a/cpp/test/format/lance/lance_bridge_error_test.cpp b/cpp/test/format/lance/lance_bridge_error_test.cpp index e71b4c256..ccae3c8f8 100644 --- a/cpp/test/format/lance/lance_bridge_error_test.cpp +++ b/cpp/test/format/lance/lance_bridge_error_test.cpp @@ -96,6 +96,22 @@ TEST(BridgeErrorTest, TranslatePreservesClassificationAndAddsContext) { EXPECT_EQ(decoded.message().find("__LOON_"), std::string::npos); } +TEST(BridgeErrorTest, TranslateDoesNotDowngradeNonIOErrorStatuses) { + // Bridge errors only travel as IOError strings; statuses arrow itself + // produced (Invalid / OutOfMemory from ImportChunkedArray etc.) must pass + // through with their StatusCode intact -- re-decoding them would downgrade + // OOM (retriable 2034) into StorageError (non-retriable 2044). + auto invalid = TranslateBridgeStatus("ctx", arrow::Status::Invalid("bad schema")); + EXPECT_TRUE(invalid.IsInvalid()) << invalid.ToString(); + + auto oom = TranslateBridgeStatus("ctx", arrow::Status::OutOfMemory("alloc failed")); + ASSERT_TRUE(oom.IsOutOfMemory()) << oom.ToString(); + EXPECT_EQ(ToSegcoreError(oom).get_error_code(), milvus::MemAllocateFailed); + + auto not_impl = TranslateBridgeStatus("ctx", arrow::Status::NotImplemented("nope")); + EXPECT_TRUE(not_impl.IsNotImplemented()) << not_impl.ToString(); +} + // End-to-end: a real lance open against a nonexistent local dataset must come // back as a classified not-found (ENOENT detail -> ObjectNotExist), not as an // exception and not as an opaque IOError. diff --git a/cpp/test/format/lance/lance_table_test.cpp b/cpp/test/format/lance/lance_table_test.cpp index c86ca9cbd..b877e6372 100644 --- a/cpp/test/format/lance/lance_table_test.cpp +++ b/cpp/test/format/lance/lance_table_test.cpp @@ -13,6 +13,10 @@ // limitations under the License. #include +#include +#include + +#include #include #include #include @@ -280,6 +284,49 @@ TEST_F(LanceBasicTest, DefaultStorageVersionIsV2_1) { ASSERT_EQ(storage_version.minor, 1); } +// Anchors the create-if-missing decision of LanceTableWriter::Close(): a +// classified not-found (and only that) creates a fresh dataset. +TEST_F(LanceBasicTest, CloseCreatesDatasetOnClassifiedNotFound) { + if (IsCloudEnv()) { + GTEST_SKIP() << "local-filesystem test"; + } + LanceTableWriter writer(base_path_ + "/fresh-dataset", schema_, properties_); + ASSERT_STATUS_OK(writer.Write(test_batch_)); + ASSERT_AND_ASSIGN(auto cgfile, writer.Close()); + ASSERT_EQ(cgfile.end_index, test_batch_->num_rows()); +} + +// The other direction of the same decision -- the core semantic change of the +// bridge-classification PR: an open failure that is NOT a classified not-found +// (here: EACCES) must propagate instead of silently creating a new dataset, +// as the old catch(std::exception) fallback did. +TEST_F(LanceBasicTest, CloseDoesNotCreateDatasetOnNonNotFoundOpenError) { + if (IsCloudEnv()) { + GTEST_SKIP() << "local-filesystem test"; + } + if (::geteuid() == 0) { + GTEST_SKIP() << "permission bits are ineffective for root"; + } + auto* subtree = dynamic_cast(fs_.get()); + ASSERT_NE(subtree, nullptr); + const boost::filesystem::path denied_dir = + boost::filesystem::path(subtree->base_path()) / arrow_base_path_ / "denied"; + boost::filesystem::create_directories(denied_dir / "ds"); + ::chmod(denied_dir.string().c_str(), 0000); + + LanceTableWriter writer(base_path_ + "/denied/ds", schema_, properties_); + ASSERT_STATUS_OK(writer.Write(test_batch_)); + auto close_result = writer.Close(); + + ::chmod(denied_dir.string().c_str(), 0755); // restore before asserting so TearDown can clean up + + ASSERT_FALSE(close_result.ok()); + const auto& status = close_result.status(); + EXPECT_NE(arrow::internal::ErrnoFromStatus(status), ENOENT) << status.ToString(); + // No dataset must have been created behind the failure. + EXPECT_FALSE(boost::filesystem::exists(denied_dir / "ds" / "_versions")) << "dataset was created despite the error"; +} + TEST_F(LanceBasicTest, TestBasic) { size_t num_of_batches = 10; if (IsCloudEnv()) { From c3a7ac76f3216f143ab396797687392f885a6f7e Mon Sep 17 00:00:00 2001 From: "zhenshan.cao" Date: Mon, 27 Jul 2026 18:40:25 -0700 Subject: [PATCH 3/9] address round-2 review: decode by marker presence; classify credential-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) Signed-off-by: zhenshan.cao --- .../bridge/rust/src/aliyun_oss_provider.rs | 12 +++++- .../format/bridge/rust/src/bridge_error.cpp | 17 +++++--- .../format/bridge/rust/src/bridge_error.rs | 12 ++++++ .../bridge/rust/src/gcp_impersonation.rs | 43 +++++++++++++------ .../format/lance/lance_bridge_error_test.cpp | 17 ++++++++ 5 files changed, 79 insertions(+), 22 deletions(-) diff --git a/cpp/src/format/bridge/rust/src/aliyun_oss_provider.rs b/cpp/src/format/bridge/rust/src/aliyun_oss_provider.rs index 039594527..83e819468 100644 --- a/cpp/src/format/bridge/rust/src/aliyun_oss_provider.rs +++ b/cpp/src/format/bridge/rust/src/aliyun_oss_provider.rs @@ -400,7 +400,12 @@ async fn call_assume_role_with_oidc( .await .map_err(|e| format!("AssumeRoleWithOIDC body read: {e}"))?; if !status.is_success() { - return Err(format!("AssumeRoleWithOIDC HTTP {status}: {text}")); + // Canonical pattern so classify_http_status_in_message can recover the + // class (429/503 transient, 403 auth) downstream. + return Err(format!( + "AssumeRoleWithOIDC failed: non-2xx status code: {}: {text}", + status.as_u16() + )); } #[derive(serde::Deserialize)] @@ -1295,7 +1300,10 @@ pub(crate) mod ram { .await .map_err(|e| format!("sts:AssumeRole body read: {e}"))?; if !status.is_success() { - return Err(format!("sts:AssumeRole HTTP {status}: {text}")); + return Err(format!( + "sts:AssumeRole failed: non-2xx status code: {}: {text}", + status.as_u16() + )); } #[derive(Deserialize)] diff --git a/cpp/src/format/bridge/rust/src/bridge_error.cpp b/cpp/src/format/bridge/rust/src/bridge_error.cpp index edb677501..0542b8dc7 100644 --- a/cpp/src/format/bridge/rust/src/bridge_error.cpp +++ b/cpp/src/format/bridge/rust/src/bridge_error.cpp @@ -138,12 +138,17 @@ arrow::Status TranslateBridgeStatus(std::string_view context, const arrow::Statu if (status.ok()) { return status; } - if (ExtendStatusDetail::UnwrapStatus(status) || arrow::internal::ErrnoFromStatus(status) == ENOENT || - !status.IsIOError()) { - // Already structured, or not a bridge-encoded carrier at all: bridge - // errors only ever surface as IOError strings (arrow FFI stringification), - // so re-decoding e.g. an Invalid or OutOfMemory from arrow itself would - // DOWNGRADE its StatusCode to IOError. Pass those through untouched. + if (ExtendStatusDetail::UnwrapStatus(status) || arrow::internal::ErrnoFromStatus(status) == ENOENT) { + // Already structured -- nothing to decode. + return WithBridgeContext(context, status); + } + // Discriminate on MARKER PRESENCE, not on StatusCode: mid-scan stream + // errors arrive as whatever code the arrow C-stream import assigns (the + // exporter maps Rust errors to EINVAL, so they surface as Invalid, NOT + // IOError), still carrying the marker. Statuses without the marker (arrow's + // own Invalid / OutOfMemory / NotImplemented) pass through untouched so + // their StatusCode is never downgraded. + if (status.message().find(kBridgeErrCodeMarker) == std::string::npos) { return WithBridgeContext(context, status); } return WithBridgeContext(context, MakeBridgeErrorStatus(status.message())); diff --git a/cpp/src/format/bridge/rust/src/bridge_error.rs b/cpp/src/format/bridge/rust/src/bridge_error.rs index 823d2ef35..8573a5021 100644 --- a/cpp/src/format/bridge/rust/src/bridge_error.rs +++ b/cpp/src/format/bridge/rust/src/bridge_error.rs @@ -160,6 +160,7 @@ fn classify_http_status_in_message(msg: &str) -> Option { .take_while(|c| c.is_ascii_digit()) .collect(); match digits.parse::().ok()? { + 401 | 403 => Some(LOON_AWS_ERROR_ACCESS_DENIED), 408 => Some(LOON_TRANSIENT_TIMEOUT), 429 => Some(LOON_TRANSIENT_THROTTLING), 500 | 502 | 503 | 504 => Some(LOON_TRANSIENT_SERVICE), @@ -217,6 +218,17 @@ mod tests { source: "Server returned non-2xx status code: 503: unavailable".into(), }); assert_eq!(classify_lance_error(&e503), Some(LOON_TRANSIENT_SERVICE)); + + // Credential-path auth failures (canonical pattern emitted by the + // gcp/aliyun providers) map to access-denied, not transient. + let e403 = LanceError::from(object_store::Error::Generic { + store: "S3", + source: "sts:AssumeRole failed: non-2xx status code: 403: forbidden".into(), + }); + assert_eq!( + classify_lance_error(&e403), + Some(LOON_AWS_ERROR_ACCESS_DENIED) + ); } #[test] diff --git a/cpp/src/format/bridge/rust/src/gcp_impersonation.rs b/cpp/src/format/bridge/rust/src/gcp_impersonation.rs index 6d203d192..0102933ca 100644 --- a/cpp/src/format/bridge/rust/src/gcp_impersonation.rs +++ b/cpp/src/format/bridge/rust/src/gcp_impersonation.rs @@ -164,13 +164,22 @@ async fn fetch_impersonated_access_token( .send() .await .and_then(|r| r.error_for_status()) - .map_err(|e| object_store::Error::Generic { - store: IMPERSONATION_STORE_NAME, - source: format!( - "metadata server token request failed (this code path requires running on a \ - GCE VM with a default service account attached): {e}" - ) - .into(), + .map_err(|e| { + // Prefix the canonical pattern while the typed StatusCode is in + // hand, so classify_http_status_in_message (bridge_error.rs) can + // recover transience/auth class downstream. + let status_prefix = e + .status() + .map(|s| format!("non-2xx status code: {}: ", s.as_u16())) + .unwrap_or_default(); + object_store::Error::Generic { + store: IMPERSONATION_STORE_NAME, + source: format!( + "{status_prefix}metadata server token request failed (this code path requires \ + running on a GCE VM with a default service account attached): {e}" + ) + .into(), + } })?; let vm_token: MetadataTokenResponse = vm_resp.json().await.map_err(|e| object_store::Error::Generic { store: IMPERSONATION_STORE_NAME, @@ -191,13 +200,19 @@ async fn fetch_impersonated_access_token( .send() .await .and_then(|r| r.error_for_status()) - .map_err(|e| object_store::Error::Generic { - store: IMPERSONATION_STORE_NAME, - source: format!( - "IAM generateAccessToken({target_sa}) failed (the VM SA likely lacks \ - roles/iam.serviceAccountTokenCreator on the target SA): {e}" - ) - .into(), + .map_err(|e| { + let status_prefix = e + .status() + .map(|s| format!("non-2xx status code: {}: ", s.as_u16())) + .unwrap_or_default(); + object_store::Error::Generic { + store: IMPERSONATION_STORE_NAME, + source: format!( + "{status_prefix}IAM generateAccessToken({target_sa}) failed (the VM SA likely \ + lacks roles/iam.serviceAccountTokenCreator on the target SA): {e}" + ) + .into(), + } })?; iam_resp.json().await.map_err(|e| object_store::Error::Generic { store: IMPERSONATION_STORE_NAME, diff --git a/cpp/test/format/lance/lance_bridge_error_test.cpp b/cpp/test/format/lance/lance_bridge_error_test.cpp index ccae3c8f8..ce740cf91 100644 --- a/cpp/test/format/lance/lance_bridge_error_test.cpp +++ b/cpp/test/format/lance/lance_bridge_error_test.cpp @@ -96,6 +96,23 @@ TEST(BridgeErrorTest, TranslatePreservesClassificationAndAddsContext) { EXPECT_EQ(decoded.message().find("__LOON_"), std::string::npos); } +TEST(BridgeErrorTest, TranslateDecodesMarkerRegardlessOfStatusCode) { + // Mid-scan stream errors surface as whatever code the arrow C-stream import + // assigns (the exporter maps Rust errors to EINVAL => Invalid, not IOError) + // while still carrying the marker. Discrimination must be on marker + // presence: a marker inside an Invalid must decode to the tagged class. + auto midscan = TranslateBridgeStatus("stream", arrow::Status::Invalid(std::string(kMarker) + "109; throttled")); + auto detail = ExtendStatusDetail::UnwrapStatus(midscan); + ASSERT_NE(detail, nullptr) << midscan.ToString(); + EXPECT_EQ(detail->code(), ExtendStatusCode::StorageTransientThrottling); + EXPECT_TRUE(detail->retryable()); + EXPECT_EQ(midscan.message().find("__LOON_"), std::string::npos) << midscan.ToString(); + EXPECT_EQ(ToSegcoreError(midscan).get_error_code(), milvus::StorageTransientError); + + auto midscan_notfound = TranslateBridgeStatus("stream", arrow::Status::Invalid(std::string(kMarker) + "12; gone")); + EXPECT_EQ(arrow::internal::ErrnoFromStatus(midscan_notfound), ENOENT) << midscan_notfound.ToString(); +} + TEST(BridgeErrorTest, TranslateDoesNotDowngradeNonIOErrorStatuses) { // Bridge errors only travel as IOError strings; statuses arrow itself // produced (Invalid / OutOfMemory from ImportChunkedArray etc.) must pass From 1e0202fab62911da926a8d971d35c78e1ca13312 Mon Sep 17 00:00:00 2001 From: "zhenshan.cao" Date: Mon, 27 Jul 2026 23:46:03 -0700 Subject: [PATCH 4/9] rename the bridge error marker to __LOON_RUST_BRIDGE_ERRCODE__ (review) 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) Signed-off-by: zhenshan.cao --- .../format/bridge/rust/include/bridge_error.h | 2 +- .../format/bridge/rust/src/bridge_error.cpp | 2 +- .../format/bridge/rust/src/bridge_error.rs | 2 +- .../format/bridge/rust/src/filesystem_c.rs | 4 +-- .../format/lance/lance_bridge_error_test.cpp | 2 +- cpp/test/format/vortex/vortex_basic_test.cpp | 30 +++++++++---------- 6 files changed, 21 insertions(+), 21 deletions(-) diff --git a/cpp/src/format/bridge/rust/include/bridge_error.h b/cpp/src/format/bridge/rust/include/bridge_error.h index ddaf3a20e..a32c8b90a 100644 --- a/cpp/src/format/bridge/rust/include/bridge_error.h +++ b/cpp/src/format/bridge/rust/include/bridge_error.h @@ -27,7 +27,7 @@ namespace milvus_storage::bridge { // // The cxx boundary only carries an error as a display string; the Rust side // (rust/src/bridge_error.rs, vortex's filesystem_c.rs) embeds the -// classification as "__LOON_VORTEX_FFI_ERRCODE__=; message". The +// classification as "__LOON_RUST_BRIDGE_ERRCODE__=; message". The // helpers here parse and strip that marker and rebuild a structured // arrow::Status: // * code 12 (LOON_FILE_NOT_FOUND) -> IOError + ENOENT detail diff --git a/cpp/src/format/bridge/rust/src/bridge_error.cpp b/cpp/src/format/bridge/rust/src/bridge_error.cpp index 0542b8dc7..4f49d1014 100644 --- a/cpp/src/format/bridge/rust/src/bridge_error.cpp +++ b/cpp/src/format/bridge/rust/src/bridge_error.cpp @@ -30,7 +30,7 @@ namespace { // One marker, one parser: must stay byte-identical to the constants in // rust/src/bridge_error.rs and rust/src/filesystem_c.rs. -constexpr std::string_view kBridgeErrCodeMarker = "__LOON_VORTEX_FFI_ERRCODE__="; +constexpr std::string_view kBridgeErrCodeMarker = "__LOON_RUST_BRIDGE_ERRCODE__="; struct ParsedBridgeError { std::string message; diff --git a/cpp/src/format/bridge/rust/src/bridge_error.rs b/cpp/src/format/bridge/rust/src/bridge_error.rs index 8573a5021..dfdc6b932 100644 --- a/cpp/src/format/bridge/rust/src/bridge_error.rs +++ b/cpp/src/format/bridge/rust/src/bridge_error.rs @@ -39,7 +39,7 @@ use lance::Error as LanceError; /// Must stay byte-identical to the vortex marker in `filesystem_c.rs` and the /// parser constant in cpp `bridge_error.cpp` — one marker, one parser. -pub const BRIDGE_ERRCODE_MARKER: &str = "__LOON_VORTEX_FFI_ERRCODE__="; +pub const BRIDGE_ERRCODE_MARKER: &str = "__LOON_RUST_BRIDGE_ERRCODE__="; /// Mirrors LOON_FILE_NOT_FOUND in `ffi_error_code.h`. pub const LOON_FILE_NOT_FOUND: i32 = 12; diff --git a/cpp/src/format/bridge/rust/src/filesystem_c.rs b/cpp/src/format/bridge/rust/src/filesystem_c.rs index 6d4faadab..c42332c20 100644 --- a/cpp/src/format/bridge/rust/src/filesystem_c.rs +++ b/cpp/src/format/bridge/rust/src/filesystem_c.rs @@ -289,7 +289,7 @@ unsafe extern "C" { ) -> LoonFFIResult; } -const LOON_VORTEX_FFI_ERRCODE_MARKER: &str = "__LOON_VORTEX_FFI_ERRCODE__="; +const LOON_RUST_BRIDGE_ERRCODE_MARKER: &str = "__LOON_RUST_BRIDGE_ERRCODE__="; #[derive(Debug)] struct LoonFfiError { @@ -302,7 +302,7 @@ impl std::fmt::Display for LoonFfiError { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { write!( f, - "{LOON_VORTEX_FFI_ERRCODE_MARKER}{}; {}: {}", + "{LOON_RUST_BRIDGE_ERRCODE_MARKER}{}; {}: {}", self.err_code, self.context, self.message ) } diff --git a/cpp/test/format/lance/lance_bridge_error_test.cpp b/cpp/test/format/lance/lance_bridge_error_test.cpp index ce740cf91..461a71a9c 100644 --- a/cpp/test/format/lance/lance_bridge_error_test.cpp +++ b/cpp/test/format/lance/lance_bridge_error_test.cpp @@ -33,7 +33,7 @@ namespace milvus_storage::bridge { namespace { -constexpr const char* kMarker = "__LOON_VORTEX_FFI_ERRCODE__="; +constexpr const char* kMarker = "__LOON_RUST_BRIDGE_ERRCODE__="; TEST(BridgeErrorTest, NotFoundCodeBecomesEnoentDetail) { auto status = MakeBridgeErrorStatus(std::string(kMarker) + "12; dataset was not found"); diff --git a/cpp/test/format/vortex/vortex_basic_test.cpp b/cpp/test/format/vortex/vortex_basic_test.cpp index 50495bd4b..09a99d453 100644 --- a/cpp/test/format/vortex/vortex_basic_test.cpp +++ b/cpp/test/format/vortex/vortex_basic_test.cpp @@ -176,7 +176,7 @@ void AsyncScanTestCallback(void* raw_ctx, ArrowArrayStream* out_stream, const ch TEST(VortexErrorTest, StreamingReaderTranslatesReadNextBridgeError) { auto inner = std::make_shared( - arrow::Status::IOError(fmt::format("__LOON_VORTEX_FFI_ERRCODE__={}; readat failed", LOON_TRANSIENT_NETWORK))); + arrow::Status::IOError(fmt::format("__LOON_RUST_BRIDGE_ERRCODE__={}; readat failed", LOON_TRANSIENT_NETWORK))); auto reader = vortex::internal::WrapVortexRecordBatchReader(std::move(inner)); std::shared_ptr batch; @@ -186,12 +186,12 @@ TEST(VortexErrorTest, StreamingReaderTranslatesReadNextBridgeError) { ASSERT_NE(detail, nullptr) << status.ToString(); EXPECT_EQ(detail->code(), ExtendStatusCode::StorageTransientNetwork); EXPECT_TRUE(detail->retryable()); - EXPECT_EQ(status.ToString().find("__LOON_VORTEX_FFI_ERRCODE__"), std::string::npos); + EXPECT_EQ(status.ToString().find("__LOON_RUST_BRIDGE_ERRCODE__"), std::string::npos); } TEST(VortexErrorTest, StreamingReaderTranslatesReadNextFileNotFound) { auto inner = std::make_shared( - arrow::Status::IOError(fmt::format("__LOON_VORTEX_FFI_ERRCODE__={}; file not found", LOON_FILE_NOT_FOUND))); + arrow::Status::IOError(fmt::format("__LOON_RUST_BRIDGE_ERRCODE__={}; file not found", LOON_FILE_NOT_FOUND))); auto reader = vortex::internal::WrapVortexRecordBatchReader(std::move(inner)); std::shared_ptr batch; @@ -200,12 +200,12 @@ TEST(VortexErrorTest, StreamingReaderTranslatesReadNextFileNotFound) { EXPECT_TRUE(status.IsIOError()); EXPECT_EQ(arrow::internal::ErrnoFromStatus(status), ENOENT); EXPECT_EQ(ExtendStatusDetail::UnwrapStatus(status), nullptr); - EXPECT_EQ(status.ToString().find("__LOON_VORTEX_FFI_ERRCODE__"), std::string::npos); + EXPECT_EQ(status.ToString().find("__LOON_RUST_BRIDGE_ERRCODE__"), std::string::npos); } TEST(VortexErrorTest, StreamingReaderTranslatesCloseBridgeError) { auto inner = std::make_shared( - arrow::Status::IOError(fmt::format("__LOON_VORTEX_FFI_ERRCODE__={}; close failed", LOON_TRANSIENT_TIMEOUT))); + arrow::Status::IOError(fmt::format("__LOON_RUST_BRIDGE_ERRCODE__={}; close failed", LOON_TRANSIENT_TIMEOUT))); auto reader = vortex::internal::WrapVortexRecordBatchReader(std::move(inner)); auto status = reader->Close(); @@ -214,27 +214,27 @@ TEST(VortexErrorTest, StreamingReaderTranslatesCloseBridgeError) { ASSERT_NE(detail, nullptr) << status.ToString(); EXPECT_EQ(detail->code(), ExtendStatusCode::StorageTransientTimeout); EXPECT_TRUE(detail->retryable()); - EXPECT_EQ(status.ToString().find("__LOON_VORTEX_FFI_ERRCODE__"), std::string::npos); + EXPECT_EQ(status.ToString().find("__LOON_RUST_BRIDGE_ERRCODE__"), std::string::npos); } TEST(VortexErrorTest, MapsBridgeErrorCodesToStatusDetails) { auto file_not_found_status = MakeVortexErrorStatus( - "Failed to read vortex file", fmt::format("__LOON_VORTEX_FFI_ERRCODE__={}; file not found", LOON_FILE_NOT_FOUND)); + "Failed to read vortex file", fmt::format("__LOON_RUST_BRIDGE_ERRCODE__={}; file not found", LOON_FILE_NOT_FOUND)); EXPECT_TRUE(file_not_found_status.IsIOError()); EXPECT_EQ(arrow::internal::ErrnoFromStatus(file_not_found_status), ENOENT); EXPECT_EQ(ExtendStatusDetail::UnwrapStatus(file_not_found_status), nullptr); - EXPECT_EQ(file_not_found_status.ToString().find("__LOON_VORTEX_FFI_ERRCODE__"), std::string::npos); + EXPECT_EQ(file_not_found_status.ToString().find("__LOON_RUST_BRIDGE_ERRCODE__"), std::string::npos); auto aws_not_found_status = MakeVortexErrorStatus("Failed to read vortex file", - fmt::format("__LOON_VORTEX_FFI_ERRCODE__={}; object not found", LOON_AWS_ERROR_NOT_FOUND)); + fmt::format("__LOON_RUST_BRIDGE_ERRCODE__={}; object not found", LOON_AWS_ERROR_NOT_FOUND)); auto aws_not_found_detail = ExtendStatusDetail::UnwrapStatus(aws_not_found_status); ASSERT_NE(aws_not_found_detail, nullptr); EXPECT_EQ(aws_not_found_detail->code(), ExtendStatusCode::AwsErrorNotFound); EXPECT_FALSE(aws_not_found_detail->retryable()); auto timeout_status = MakeVortexErrorStatus( - "Failed to read vortex file", fmt::format("__LOON_VORTEX_FFI_ERRCODE__={}; read failed", LOON_TRANSIENT_TIMEOUT)); + "Failed to read vortex file", fmt::format("__LOON_RUST_BRIDGE_ERRCODE__={}; read failed", LOON_TRANSIENT_TIMEOUT)); auto timeout_detail = ExtendStatusDetail::UnwrapStatus(timeout_status); ASSERT_NE(timeout_detail, nullptr); EXPECT_EQ(timeout_detail->code(), ExtendStatusCode::StorageTransientTimeout); @@ -243,26 +243,26 @@ TEST(VortexErrorTest, MapsBridgeErrorCodesToStatusDetails) { auto upload_status = MakeVortexErrorStatus("Failed to close Vortex file", - fmt::format("outer __LOON_VORTEX_FFI_ERRCODE__={}; Failed to close ObjectStoreWriterCpp", + fmt::format("outer __LOON_RUST_BRIDGE_ERRCODE__={}; Failed to close ObjectStoreWriterCpp", LOON_AWS_ERROR_NO_SUCH_UPLOAD)); auto upload_detail = ExtendStatusDetail::UnwrapStatus(upload_status); ASSERT_NE(upload_detail, nullptr); EXPECT_EQ(upload_detail->code(), ExtendStatusCode::AwsErrorNoSuchUpload); EXPECT_TRUE(upload_detail->retryable()); - EXPECT_EQ(upload_status.ToString().find("__LOON_VORTEX_FFI_ERRCODE__"), std::string::npos); + EXPECT_EQ(upload_status.ToString().find("__LOON_RUST_BRIDGE_ERRCODE__"), std::string::npos); auto network_status = MakeVortexErrorStatus( "Failed to import vortex chunked array", - arrow::Status::IOError(fmt::format("__LOON_VORTEX_FFI_ERRCODE__={}; readat failed", LOON_TRANSIENT_NETWORK))); + arrow::Status::IOError(fmt::format("__LOON_RUST_BRIDGE_ERRCODE__={}; readat failed", LOON_TRANSIENT_NETWORK))); auto network_detail = ExtendStatusDetail::UnwrapStatus(network_status); ASSERT_NE(network_detail, nullptr); EXPECT_EQ(network_detail->code(), ExtendStatusCode::StorageTransientNetwork); EXPECT_TRUE(network_detail->retryable()); - EXPECT_EQ(network_status.ToString().find("__LOON_VORTEX_FFI_ERRCODE__"), std::string::npos); + EXPECT_EQ(network_status.ToString().find("__LOON_RUST_BRIDGE_ERRCODE__"), std::string::npos); auto txn_status = MakeVortexErrorStatus("Failed to write Vortex file", - fmt::format("__LOON_VORTEX_FFI_ERRCODE__={}; commit failed", LOON_TXN_EXHAUSTED_RETRY)); + fmt::format("__LOON_RUST_BRIDGE_ERRCODE__={}; commit failed", LOON_TXN_EXHAUSTED_RETRY)); auto txn_detail = ExtendStatusDetail::UnwrapStatus(txn_status); ASSERT_NE(txn_detail, nullptr); EXPECT_EQ(txn_detail->code(), ExtendStatusCode::TxnExhaustedRetry); From bcb9feed4b6575614a51fd926fe963bfdef5eb8e Mon Sep 17 00:00:00 2001 From: xiaofanluan Date: Wed, 29 Jul 2026 14:10:04 -0700 Subject: [PATCH 5/9] fix: preserve unclassified Lance bridge errors Signed-off-by: xiaofanluan --- .github/workflows/cpp-ci.yml | 8 ++++ .../format/bridge/rust/include/bridge_error.h | 2 + .../format/bridge/rust/src/bridge_error.cpp | 5 +++ .../format/bridge/rust/src/bridge_error.rs | 41 ++++++++++++++----- cpp/src/format/lance/lance_table_reader.cpp | 3 +- .../format/lance/lance_bridge_error_test.cpp | 11 +++++ cpp/test/format/vortex/vortex_basic_test.cpp | 10 +++-- cpp/tools/loon.cpp | 13 +++--- 8 files changed, 73 insertions(+), 20 deletions(-) diff --git a/.github/workflows/cpp-ci.yml b/.github/workflows/cpp-ci.yml index f4326977c..94f0dfc15 100644 --- a/.github/workflows/cpp-ci.yml +++ b/.github/workflows/cpp-ci.yml @@ -67,6 +67,14 @@ jobs: run: | make build USE_ASAN=True BUILD_TYPE=Release + - name: Test Rust bridge error classifier + working-directory: ./cpp + env: + CARGO_TARGET_DIR: ${{ github.workspace }}/cpp/build/Release/cargo/build + RUSTFLAGS: -C force-frame-pointers=yes + run: | + cargo test --locked --manifest-path src/format/bridge/rust/Cargo.toml --lib bridge_error::tests + - name: Save rust build cache if: github.ref == 'refs/heads/main' && steps.rust-cache.outputs.cache-hit != 'true' uses: actions/cache/save@v4 diff --git a/cpp/src/format/bridge/rust/include/bridge_error.h b/cpp/src/format/bridge/rust/include/bridge_error.h index a32c8b90a..b99429d54 100644 --- a/cpp/src/format/bridge/rust/include/bridge_error.h +++ b/cpp/src/format/bridge/rust/include/bridge_error.h @@ -33,12 +33,14 @@ namespace milvus_storage::bridge { // * code 12 (LOON_FILE_NOT_FOUND) -> IOError + ENOENT detail // * ExtendStatusCode values (101-112) -> IOError + ExtendStatusDetail // * bridge-private codes (>= 1000, never cross the C ABI): +// 1000 unclassified -> plain IOError (conservative fallback) // 1001 data-corrupt -> Status::Invalid (permanent data error) // 1002 not-supported -> Status::NotImplemented // * no / unknown marker -> plain IOError (conservative // non-retriable fallback; never invent retriability) // Bridge-private marker codes; keep in sync with rust/src/bridge_error.rs. +inline constexpr int kBridgeErrCodeUnclassified = 1000; inline constexpr int kBridgeErrCodeDataCorrupt = 1001; inline constexpr int kBridgeErrCodeNotSupported = 1002; diff --git a/cpp/src/format/bridge/rust/src/bridge_error.cpp b/cpp/src/format/bridge/rust/src/bridge_error.cpp index 4f49d1014..15aa4aa08 100644 --- a/cpp/src/format/bridge/rust/src/bridge_error.cpp +++ b/cpp/src/format/bridge/rust/src/bridge_error.cpp @@ -106,6 +106,11 @@ arrow::Status MakeBridgeErrorStatus(std::string_view message) { switch (*parsed.ffi_err_code) { case LOON_FILE_NOT_FOUND: return arrow::Status::IOError(parsed.message).WithDetail(arrow::internal::StatusDetailFromErrno(ENOENT)); + case kBridgeErrCodeUnclassified: + // Explicit marker used by Rust when no more specific classification is + // available. It must still be decoded so Arrow C-stream's Invalid/EINVAL + // wrapper does not turn an opaque IO failure into DataFormatBroken. + break; case kBridgeErrCodeDataCorrupt: return arrow::Status::Invalid(parsed.message); case kBridgeErrCodeNotSupported: diff --git a/cpp/src/format/bridge/rust/src/bridge_error.rs b/cpp/src/format/bridge/rust/src/bridge_error.rs index dfdc6b932..2e7a3210e 100644 --- a/cpp/src/format/bridge/rust/src/bridge_error.rs +++ b/cpp/src/format/bridge/rust/src/bridge_error.rs @@ -29,11 +29,14 @@ //! matching `ExtendStatusDetail` (or an ENOENT detail for 12). //! * Bridge-private values (>= 1000, never cross the C ABI): the C++ side //! converts them straight into an arrow StatusCode and they cease to exist. +//! Code 1000 is the explicit unclassified fallback; carrying it is important +//! for stream errors because Arrow's C stream maps every Rust error to +//! `Invalid` before C++ gets a chance to restore the original class. //! //! Classification discipline ("producer owns classification", conservative): -//! only signals the producer positively identifies are tagged; everything else -//! stays untagged and lands in the consumer's non-retriable fallback bucket. -//! Never invent retriability. +//! only signals the producer positively identifies get a semantic code; +//! everything else carries the explicit unclassified marker and lands in the +//! consumer's non-retriable fallback bucket. Never invent retriability. use lance::Error as LanceError; @@ -52,6 +55,7 @@ pub const LOON_TRANSIENT_SERVICE: i32 = 110; /// Bridge-private codes (>= 1000): decoded by cpp `bridge_error.cpp` into an /// arrow StatusCode, never forwarded as an FFI error code. +pub const BRIDGE_ERRCODE_UNCLASSIFIED: i32 = 1000; pub const BRIDGE_ERRCODE_DATA_CORRUPT: i32 = 1001; pub const BRIDGE_ERRCODE_NOT_SUPPORTED: i32 = 1002; @@ -65,10 +69,13 @@ pub struct BridgeError { impl std::fmt::Display for BridgeError { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - match self.code { - Some(code) => write!(f, "{BRIDGE_ERRCODE_MARKER}{code}; {}", self.msg), - None => write!(f, "{}", self.msg), - } + // Always carry a marker. In synchronous cxx calls an unmarked error + // would still become a plain IOError, but during Arrow C-stream + // iteration it first becomes Invalid/EINVAL. The explicit fallback + // marker lets the C++ decoder restore that stream error to IOError + // instead of misreporting it as DataFormatBroken. + let code = self.code.unwrap_or(BRIDGE_ERRCODE_UNCLASSIFIED); + write!(f, "{BRIDGE_ERRCODE_MARKER}{code}; {}", self.msg) } } @@ -79,9 +86,9 @@ impl std::error::Error for BridgeError {} /// impls below. pub type BridgeResult = std::result::Result; -/// Classify a `lance::Error` into a marker code. `None` = not positively -/// identified -> stays untagged -> conservative non-retriable fallback on the -/// consumer side. +/// Classify a `lance::Error` into a semantic marker code. `None` = not +/// positively identified; `Display` emits the explicit unclassified marker so +/// the consumer can still restore the conservative non-retriable IO fallback. pub fn classify_lance_error(e: &LanceError) -> Option { match e { // The object/dataset/index/ref/version is gone. Retrying hits the same @@ -246,6 +253,20 @@ mod tests { assert_eq!(classify_lance_error(&e404ish), None); } + #[test] + fn unclassified_errors_still_carry_the_bridge_marker() { + let error = BridgeError { + code: None, + msg: "connection reset by peer".to_string(), + }; + assert_eq!( + error.to_string(), + format!( + "{BRIDGE_ERRCODE_MARKER}{BRIDGE_ERRCODE_UNCLASSIFIED}; connection reset by peer" + ) + ); + } + #[test] fn field_and_schema_errors_are_not_enoent() { // FieldNotFound must never classify as file-not-found: ENOENT drives diff --git a/cpp/src/format/lance/lance_table_reader.cpp b/cpp/src/format/lance/lance_table_reader.cpp index 09d243cf2..5896c3554 100644 --- a/cpp/src/format/lance/lance_table_reader.cpp +++ b/cpp/src/format/lance/lance_table_reader.cpp @@ -468,7 +468,8 @@ arrow::Result> LanceTableReader::take(const std::v // out of range if (chunkedarray->num_chunks() == 0) { - return arrow::Status::Invalid(fmt::format("out of row range [0, {}]", fragment_reader_->RowCount().ValueOr(0))); + ARROW_ASSIGN_OR_RAISE(auto row_count, fragment_reader_->RowCount()); + return arrow::Status::Invalid(fmt::format("out of row range [0, {}]", row_count)); } std::vector> rbs; diff --git a/cpp/test/format/lance/lance_bridge_error_test.cpp b/cpp/test/format/lance/lance_bridge_error_test.cpp index 461a71a9c..f33b9d6e3 100644 --- a/cpp/test/format/lance/lance_bridge_error_test.cpp +++ b/cpp/test/format/lance/lance_bridge_error_test.cpp @@ -111,6 +111,17 @@ TEST(BridgeErrorTest, TranslateDecodesMarkerRegardlessOfStatusCode) { auto midscan_notfound = TranslateBridgeStatus("stream", arrow::Status::Invalid(std::string(kMarker) + "12; gone")); EXPECT_EQ(arrow::internal::ErrnoFromStatus(midscan_notfound), ENOENT) << midscan_notfound.ToString(); + + // Rust now emits bridge-private code 1000 when it cannot classify the + // underlying error. Arrow C-stream exposes it as Invalid/EINVAL, but the + // marker must restore the conservative plain-IO fallback rather than + // misreporting a network/opaque failure as DataFormatBroken. + auto midscan_unclassified = + TranslateBridgeStatus("stream", arrow::Status::Invalid(std::string(kMarker) + "1000; connection reset")); + EXPECT_TRUE(midscan_unclassified.IsIOError()) << midscan_unclassified.ToString(); + EXPECT_EQ(ExtendStatusDetail::UnwrapStatus(midscan_unclassified), nullptr); + EXPECT_EQ(ToSegcoreError(midscan_unclassified).get_error_code(), milvus::StorageError); + EXPECT_EQ(midscan_unclassified.message().find("__LOON_"), std::string::npos); } TEST(BridgeErrorTest, TranslateDoesNotDowngradeNonIOErrorStatuses) { diff --git a/cpp/test/format/vortex/vortex_basic_test.cpp b/cpp/test/format/vortex/vortex_basic_test.cpp index 09a99d453..61a77dbbc 100644 --- a/cpp/test/format/vortex/vortex_basic_test.cpp +++ b/cpp/test/format/vortex/vortex_basic_test.cpp @@ -218,8 +218,9 @@ TEST(VortexErrorTest, StreamingReaderTranslatesCloseBridgeError) { } TEST(VortexErrorTest, MapsBridgeErrorCodesToStatusDetails) { - auto file_not_found_status = MakeVortexErrorStatus( - "Failed to read vortex file", fmt::format("__LOON_RUST_BRIDGE_ERRCODE__={}; file not found", LOON_FILE_NOT_FOUND)); + auto file_not_found_status = + MakeVortexErrorStatus("Failed to read vortex file", + fmt::format("__LOON_RUST_BRIDGE_ERRCODE__={}; file not found", LOON_FILE_NOT_FOUND)); EXPECT_TRUE(file_not_found_status.IsIOError()); EXPECT_EQ(arrow::internal::ErrnoFromStatus(file_not_found_status), ENOENT); EXPECT_EQ(ExtendStatusDetail::UnwrapStatus(file_not_found_status), nullptr); @@ -233,8 +234,9 @@ TEST(VortexErrorTest, MapsBridgeErrorCodesToStatusDetails) { EXPECT_EQ(aws_not_found_detail->code(), ExtendStatusCode::AwsErrorNotFound); EXPECT_FALSE(aws_not_found_detail->retryable()); - auto timeout_status = MakeVortexErrorStatus( - "Failed to read vortex file", fmt::format("__LOON_RUST_BRIDGE_ERRCODE__={}; read failed", LOON_TRANSIENT_TIMEOUT)); + auto timeout_status = + MakeVortexErrorStatus("Failed to read vortex file", + fmt::format("__LOON_RUST_BRIDGE_ERRCODE__={}; read failed", LOON_TRANSIENT_TIMEOUT)); auto timeout_detail = ExtendStatusDetail::UnwrapStatus(timeout_status); ASSERT_NE(timeout_detail, nullptr); EXPECT_EQ(timeout_detail->code(), ExtendStatusCode::StorageTransientTimeout); diff --git a/cpp/tools/loon.cpp b/cpp/tools/loon.cpp index f4dcdd9a0..8df65aed0 100644 --- a/cpp/tools/loon.cpp +++ b/cpp/tools/loon.cpp @@ -151,7 +151,8 @@ static int DoDemoTable(int argc, char** argv) { std::cerr << std::endl; std::cerr << "Types: iceberg" << std::endl; std::cerr << std::endl; - std::cerr << "Creates a demo table with schema (id int64, name string," << " value float64)." << std::endl; + std::cerr << "Creates a demo table with schema (id int64, name string," + << " value float64)." << std::endl; std::cerr << R"(Data: id=0..N-1, name="row_0".."row_{N-1}", value=id*1.5)" << std::endl; std::cerr << std::endl; std::cerr << "For cloud storage, pass extfs.* properties via --prop." << std::endl; @@ -361,7 +362,8 @@ static int DoCreate(int argc, char** argv) { if (format.empty() || source.empty() || target.empty() || columns.empty()) { std::cerr << "Usage: loon create --format --source " - << "--target --columns col1,col2,... " << "[--prop key=value ...]" << std::endl; + << "--target --columns col1,col2,... " + << "[--prop key=value ...]" << std::endl; std::cerr << std::endl; std::cerr << "Formats: parquet, vortex, lance-table, iceberg-table" << std::endl; std::cerr << std::endl; @@ -567,7 +569,8 @@ static int DoDescribe(int argc, char** argv) { static int DoRead(int argc, char** argv) { if (argc < 1) { std::cerr << "Usage: loon read --columns col1,col2,..." - << " [--take pos1,pos2,...] [--predicate \"expr\"]" << " [--verbose] [--prop key=value ...]" << std::endl; + << " [--take pos1,pos2,...] [--predicate \"expr\"]" + << " [--verbose] [--prop key=value ...]" << std::endl; return 1; } std::string manifest_path = argv[0]; @@ -638,8 +641,8 @@ static int DoRead(int argc, char** argv) { for (size_t fi = 0; fi < cg->files.size(); ++fi) { auto& f = cg->files[fi]; std::cout << " file[" << fi << "] path=" << f.path << " range=[" << f.start_index << "," << f.end_index - << ")" << " has_metadata=" << (f.properties.count(kPropertyMetadata) > 0 ? "true" : "false") - << std::endl; + << ")" + << " has_metadata=" << (f.properties.count(kPropertyMetadata) > 0 ? "true" : "false") << std::endl; auto meta_it = f.properties.find(kPropertyMetadata); if (meta_it != f.properties.end()) { std::cout << " metadata: " << meta_it->second << std::endl; From cd7a578385f39ca19eaa36354786146b783174ac Mon Sep 17 00:00:00 2001 From: xiaofanluan Date: Wed, 29 Jul 2026 14:53:59 -0700 Subject: [PATCH 6/9] fix: separate Lance contention and not-found errors Co-Authored-By: Claude Opus 4.6 Signed-off-by: xiaofanluan --- cpp/ffi_exports.map | 2 + cpp/ffi_exports_mac.map | 2 + .../milvus-storage/common/extend_status.h | 5 ++ cpp/include/milvus-storage/ffi_c.h | 2 + .../ffi_internal/ffi_error_code.h | 2 + cpp/scripts/error_handling_baseline.tsv | 2 - cpp/src/common/extend_status.cpp | 4 ++ cpp/src/ffi/result_c.cpp | 4 ++ .../format/bridge/rust/include/bridge_error.h | 2 +- .../format/bridge/rust/src/bridge_error.rs | 65 +++++++++++++++++-- cpp/test/common/extend_status_test.cpp | 11 ++++ cpp/test/ffi/ffi_filesystem_test.c | 2 + .../format/lance/lance_bridge_error_test.cpp | 24 ++++++- python/milvus_storage/_ffi.py | 4 ++ 14 files changed, 118 insertions(+), 13 deletions(-) diff --git a/cpp/ffi_exports.map b/cpp/ffi_exports.map index 34a9a0769..0783fca4e 100644 --- a/cpp/ffi_exports.map +++ b/cpp/ffi_exports.map @@ -31,6 +31,8 @@ loon_errcode_transient_service; loon_errcode_txn_exhausted_retry; loon_errcode_txn_resolution_failed; + loon_errcode_lance_write_contention; + loon_errcode_lance_resource_not_found; # Properties interface loon_properties_create; diff --git a/cpp/ffi_exports_mac.map b/cpp/ffi_exports_mac.map index b92026c0d..13fd35028 100644 --- a/cpp/ffi_exports_mac.map +++ b/cpp/ffi_exports_mac.map @@ -29,6 +29,8 @@ _loon_errcode_transient_throttling _loon_errcode_transient_service _loon_errcode_txn_exhausted_retry _loon_errcode_txn_resolution_failed +_loon_errcode_lance_write_contention +_loon_errcode_lance_resource_not_found # Properties interface _loon_properties_create diff --git a/cpp/include/milvus-storage/common/extend_status.h b/cpp/include/milvus-storage/common/extend_status.h index c6698acba..878073cfb 100644 --- a/cpp/include/milvus-storage/common/extend_status.h +++ b/cpp/include/milvus-storage/common/extend_status.h @@ -58,6 +58,11 @@ enum class ExtendStatusCode : char { // Transaction-specific error codes TxnExhaustedRetry = LOON_TXN_EXHAUSTED_RETRY, TxnResolutionFailed = LOON_TXN_RESOLUTION_FAILED, + + // Lance-specific error codes. These preserve Lance semantics without + // overloading object-storage throttling or the dataset-absence ENOENT signal. + LanceWriteContention = LOON_LANCE_WRITE_CONTENTION, + LanceResourceNotFound = LOON_LANCE_RESOURCE_NOT_FOUND, }; class ExtendStatusDetail : public arrow::StatusDetail { diff --git a/cpp/include/milvus-storage/ffi_c.h b/cpp/include/milvus-storage/ffi_c.h index c30b37c64..765645548 100644 --- a/cpp/include/milvus-storage/ffi_c.h +++ b/cpp/include/milvus-storage/ffi_c.h @@ -58,6 +58,8 @@ FFI_EXPORT extern const int loon_errcode_transient_throttling; FFI_EXPORT extern const int loon_errcode_transient_service; FFI_EXPORT extern const int loon_errcode_txn_exhausted_retry; FFI_EXPORT extern const int loon_errcode_txn_resolution_failed; +FFI_EXPORT extern const int loon_errcode_lance_write_contention; +FFI_EXPORT extern const int loon_errcode_lance_resource_not_found; // usage example(caller must free the message string): // diff --git a/cpp/include/milvus-storage/ffi_internal/ffi_error_code.h b/cpp/include/milvus-storage/ffi_internal/ffi_error_code.h index ae8759b98..888c7589e 100644 --- a/cpp/include/milvus-storage/ffi_internal/ffi_error_code.h +++ b/cpp/include/milvus-storage/ffi_internal/ffi_error_code.h @@ -40,3 +40,5 @@ #define LOON_TRANSIENT_SERVICE 110 #define LOON_TXN_EXHAUSTED_RETRY 111 #define LOON_TXN_RESOLUTION_FAILED 112 +#define LOON_LANCE_WRITE_CONTENTION 113 +#define LOON_LANCE_RESOURCE_NOT_FOUND 114 diff --git a/cpp/scripts/error_handling_baseline.tsv b/cpp/scripts/error_handling_baseline.tsv index 7874fcd1c..0b03a8a55 100644 --- a/cpp/scripts/error_handling_baseline.tsv +++ b/cpp/scripts/error_handling_baseline.tsv @@ -1,8 +1,6 @@ throw cpp/src/common/metadata.cpp 4 throw cpp/src/ffi/ffi_fiu_c.cpp 1 throw cpp/src/ffi/v2_column_groups_builder.cpp 5 -throw cpp/src/format/bridge/rust/src/iceberg_bridge.cpp 2 -throw cpp/src/format/bridge/rust/src/lance_bridge.cpp 21 throw cpp/src/format/iceberg/iceberg_common.cpp 2 throw cpp/src/format/lance/lance_common.cpp 2 throw cpp/src/format/vortex/vortex_translater.cpp 6 diff --git a/cpp/src/common/extend_status.cpp b/cpp/src/common/extend_status.cpp index 44a6a249d..0eb8f6a9f 100644 --- a/cpp/src/common/extend_status.cpp +++ b/cpp/src/common/extend_status.cpp @@ -54,6 +54,8 @@ constexpr ExtendStatusCodeMetadata kExtendStatusCodeMetadata[] = { {ExtendStatusCode::StorageTransientService, "StorageTransientService", true}, {ExtendStatusCode::TxnExhaustedRetry, "TxnExhaustedRetry", false}, {ExtendStatusCode::TxnResolutionFailed, "TxnResolutionFailed", false}, + {ExtendStatusCode::LanceWriteContention, "LanceWriteContention", true}, + {ExtendStatusCode::LanceResourceNotFound, "LanceResourceNotFound", false}, }; const ExtendStatusCodeMetadata* FindExtendStatusCodeMetadata(ExtendStatusCode code) { @@ -203,6 +205,7 @@ milvus::ErrorCode ToSegcoreErrorCode(ExtendStatusCode code) { case ExtendStatusCode::StorageTransientTimeout: case ExtendStatusCode::StorageTransientThrottling: case ExtendStatusCode::StorageTransientService: + case ExtendStatusCode::LanceWriteContention: return milvus::StorageTransientError; // 2045 case ExtendStatusCode::AwsErrorConflict: case ExtendStatusCode::AwsErrorPreConditionFailed: @@ -213,6 +216,7 @@ milvus::ErrorCode ToSegcoreErrorCode(ExtendStatusCode code) { // budget is already spent). return milvus::StorageError; // 2044 case ExtendStatusCode::AwsErrorNotFound: + case ExtendStatusCode::LanceResourceNotFound: // The object/bucket is gone: permanent, and fine-grained -- consumers can // distinguish "data missing" (stale loadinfo, GC'd file) from a generic // storage failure. Never transient/2045: a retry/reroute hits the same diff --git a/cpp/src/ffi/result_c.cpp b/cpp/src/ffi/result_c.cpp index 962093827..98d3d63c9 100644 --- a/cpp/src/ffi/result_c.cpp +++ b/cpp/src/ffi/result_c.cpp @@ -44,6 +44,8 @@ extern FFI_EXPORT const int loon_errcode_transient_throttling = LOON_TRANSIENT_T extern FFI_EXPORT const int loon_errcode_transient_service = LOON_TRANSIENT_SERVICE; extern FFI_EXPORT const int loon_errcode_txn_exhausted_retry = LOON_TXN_EXHAUSTED_RETRY; extern FFI_EXPORT const int loon_errcode_txn_resolution_failed = LOON_TXN_RESOLUTION_FAILED; +extern FFI_EXPORT const int loon_errcode_lance_write_contention = LOON_LANCE_WRITE_CONTENTION; +extern FFI_EXPORT const int loon_errcode_lance_resource_not_found = LOON_LANCE_RESOURCE_NOT_FOUND; } // extern "C" @@ -72,6 +74,8 @@ std::string error_to_string(int code) { {LOON_TRANSIENT_SERVICE, "StorageTransientService"}, {LOON_TXN_EXHAUSTED_RETRY, "TxnExhaustedRetry"}, {LOON_TXN_RESOLUTION_FAILED, "TxnResolutionFailed"}, + {LOON_LANCE_WRITE_CONTENTION, "LanceWriteContention"}, + {LOON_LANCE_RESOURCE_NOT_FOUND, "LanceResourceNotFound"}, }; if (auto it = error_strings.find(code); it != error_strings.end()) { diff --git a/cpp/src/format/bridge/rust/include/bridge_error.h b/cpp/src/format/bridge/rust/include/bridge_error.h index b99429d54..c901319aa 100644 --- a/cpp/src/format/bridge/rust/include/bridge_error.h +++ b/cpp/src/format/bridge/rust/include/bridge_error.h @@ -31,7 +31,7 @@ namespace milvus_storage::bridge { // helpers here parse and strip that marker and rebuild a structured // arrow::Status: // * code 12 (LOON_FILE_NOT_FOUND) -> IOError + ENOENT detail -// * ExtendStatusCode values (101-112) -> IOError + ExtendStatusDetail +// * ExtendStatusCode values (101-114) -> IOError + ExtendStatusDetail // * bridge-private codes (>= 1000, never cross the C ABI): // 1000 unclassified -> plain IOError (conservative fallback) // 1001 data-corrupt -> Status::Invalid (permanent data error) diff --git a/cpp/src/format/bridge/rust/src/bridge_error.rs b/cpp/src/format/bridge/rust/src/bridge_error.rs index 2e7a3210e..47994bec2 100644 --- a/cpp/src/format/bridge/rust/src/bridge_error.rs +++ b/cpp/src/format/bridge/rust/src/bridge_error.rs @@ -24,8 +24,8 @@ //! established in `filesystem_c.rs`. //! //! Code space carried by the marker: -//! * LOON / ExtendStatusCode values (`ffi_error_code.h`): 12 = file-not-found, -//! 101-112 = AWS/transient/txn extend codes. The C++ side rebuilds the +//! * LOON / ExtendStatusCode values (`ffi_error_code.h`): 12 = dataset-not-found, +//! 101-114 = AWS/transient/txn/Lance extend codes. The C++ side rebuilds the //! matching `ExtendStatusDetail` (or an ENOENT detail for 12). //! * Bridge-private values (>= 1000, never cross the C ABI): the C++ side //! converts them straight into an arrow StatusCode and they cease to exist. @@ -52,6 +52,10 @@ pub const LOON_AWS_ERROR_ACCESS_DENIED: i32 = 105; pub const LOON_TRANSIENT_TIMEOUT: i32 = 108; pub const LOON_TRANSIENT_THROTTLING: i32 = 109; pub const LOON_TRANSIENT_SERVICE: i32 = 110; +/// Lance-specific ExtendStatusCode values. Keep separate from object-store +/// throttling and from code 12, which is the create-if-missing signal. +pub const LOON_LANCE_WRITE_CONTENTION: i32 = 113; +pub const LOON_LANCE_RESOURCE_NOT_FOUND: i32 = 114; /// Bridge-private codes (>= 1000): decoded by cpp `bridge_error.cpp` into an /// arrow StatusCode, never forwarded as an FFI error code. @@ -94,11 +98,14 @@ pub fn classify_lance_error(e: &LanceError) -> Option { // The object/dataset/index/ref/version is gone. Retrying hits the same // store and fails identically; consumers can distinguish "data // missing" from a generic storage failure. + LanceError::DatasetNotFound { .. } => Some(LOON_FILE_NOT_FOUND), + // These are missing resources *inside* an existing Lance dataset. They + // remain fine-grained ObjectNotExist downstream, but must not carry + // ENOENT: LanceTableWriter consumes ENOENT as "dataset absent -> create". LanceError::NotFound { .. } - | LanceError::DatasetNotFound { .. } | LanceError::IndexNotFound { .. } | LanceError::RefNotFound { .. } - | LanceError::VersionNotFound { .. } => Some(LOON_FILE_NOT_FOUND), + | LanceError::VersionNotFound { .. } => Some(LOON_LANCE_RESOURCE_NOT_FOUND), // Field/schema errors are caller/schema-evolution conditions, not // corruption -- and they must NOT look like a missing dataset: the // ENOENT classification drives create-if-missing in the lance writer, @@ -116,12 +123,15 @@ pub fn classify_lance_error(e: &LanceError) -> Option { // but a fresh attempt (new commit round) can succeed. This is the // producer's own classification, not invented here. LanceError::RetryableCommitConflict { .. } | LanceError::TooMuchWriteContention { .. } => { - Some(LOON_TRANSIENT_THROTTLING) + Some(LOON_LANCE_WRITE_CONTENTION) } // IO wraps the underlying object_store error as a boxed source; // downcast to recover the typed variant. LanceError::IO { source, .. } => match source.downcast_ref::() { - Some(object_store::Error::NotFound { .. }) => Some(LOON_FILE_NOT_FOUND), + // A missing object while operating inside a dataset is not proof + // that the dataset itself is absent. Preserve ObjectNotExist + // without emitting the writer's create-if-missing ENOENT signal. + Some(object_store::Error::NotFound { .. }) => Some(LOON_LANCE_RESOURCE_NOT_FOUND), Some( object_store::Error::PermissionDenied { .. } | object_store::Error::Unauthenticated { .. }, @@ -208,7 +218,48 @@ mod tests { path: "p".to_string(), source: "gone".into(), }); - assert_eq!(classify_lance_error(&e), Some(LOON_FILE_NOT_FOUND)); + assert_eq!( + classify_lance_error(&e), + Some(LOON_LANCE_RESOURCE_NOT_FOUND) + ); + } + + #[test] + fn dataset_not_found_is_the_only_create_if_missing_signal() { + let dataset_missing = LanceError::dataset_not_found("dataset", "gone".into()); + assert_eq!( + classify_lance_error(&dataset_missing), + Some(LOON_FILE_NOT_FOUND) + ); + + let resource_missing = LanceError::not_found("manifest"); + assert_eq!( + classify_lance_error(&resource_missing), + Some(LOON_LANCE_RESOURCE_NOT_FOUND) + ); + + let version_missing = LanceError::VersionNotFound { + message: "version 7".to_string(), + }; + assert_eq!( + classify_lance_error(&version_missing), + Some(LOON_LANCE_RESOURCE_NOT_FOUND) + ); + } + + #[test] + fn lance_contention_does_not_reuse_object_store_throttling() { + let contention = LanceError::too_much_write_contention("writers are busy"); + assert_eq!( + classify_lance_error(&contention), + Some(LOON_LANCE_WRITE_CONTENTION) + ); + + let conflict = LanceError::retryable_commit_conflict_source(7, "conflict".into()); + assert_eq!( + classify_lance_error(&conflict), + Some(LOON_LANCE_WRITE_CONTENTION) + ); } #[test] diff --git a/cpp/test/common/extend_status_test.cpp b/cpp/test/common/extend_status_test.cpp index 78dd8397c..b2596aac0 100644 --- a/cpp/test/common/extend_status_test.cpp +++ b/cpp/test/common/extend_status_test.cpp @@ -91,6 +91,8 @@ TEST_F(ExtendStatusTest, TestExtendStatusCodeRetryability) { EXPECT_EQ(ExtendStatusCodeFromInt(50), ExtendStatusCode::PackedInvalidArgs); EXPECT_EQ(ExtendStatusCodeFromInt(LOON_AWS_ERROR_NO_SUCH_UPLOAD), ExtendStatusCode::AwsErrorNoSuchUpload); EXPECT_EQ(ExtendStatusCodeFromInt(LOON_TRANSIENT_NETWORK), ExtendStatusCode::StorageTransientNetwork); + EXPECT_EQ(ExtendStatusCodeFromInt(LOON_LANCE_WRITE_CONTENTION), ExtendStatusCode::LanceWriteContention); + EXPECT_EQ(ExtendStatusCodeFromInt(LOON_LANCE_RESOURCE_NOT_FOUND), ExtendStatusCode::LanceResourceNotFound); EXPECT_FALSE(ExtendStatusCodeFromInt(3).has_value()); EXPECT_FALSE(DefaultRetryableForExtendStatusCode(ExtendStatusCode::PackedInvalidArgs)); @@ -106,6 +108,8 @@ TEST_F(ExtendStatusTest, TestExtendStatusCodeRetryability) { EXPECT_TRUE(DefaultRetryableForExtendStatusCode(ExtendStatusCode::StorageTransientService)); EXPECT_FALSE(DefaultRetryableForExtendStatusCode(ExtendStatusCode::TxnExhaustedRetry)); EXPECT_FALSE(DefaultRetryableForExtendStatusCode(ExtendStatusCode::TxnResolutionFailed)); + EXPECT_TRUE(DefaultRetryableForExtendStatusCode(ExtendStatusCode::LanceWriteContention)); + EXPECT_FALSE(DefaultRetryableForExtendStatusCode(ExtendStatusCode::LanceResourceNotFound)); auto status = MakeExtendError(ExtendStatusCode::StorageTransientNetwork, "network", "detail"); auto detail = ExtendStatusDetail::UnwrapStatus(status); @@ -121,6 +125,8 @@ TEST_F(ExtendStatusTest, TestExtendStatusDetail) { EXPECT_EQ(static_cast(ExtendStatusCode::StorageTransientTimeout), LOON_TRANSIENT_TIMEOUT); EXPECT_EQ(static_cast(ExtendStatusCode::StorageTransientThrottling), LOON_TRANSIENT_THROTTLING); EXPECT_EQ(static_cast(ExtendStatusCode::StorageTransientService), LOON_TRANSIENT_SERVICE); + EXPECT_EQ(static_cast(ExtendStatusCode::LanceWriteContention), LOON_LANCE_WRITE_CONTENTION); + EXPECT_EQ(static_cast(ExtendStatusCode::LanceResourceNotFound), LOON_LANCE_RESOURCE_NOT_FOUND); } // CodeAsString @@ -134,6 +140,8 @@ TEST_F(ExtendStatusTest, TestExtendStatusDetail) { EXPECT_EQ(ExtendStatusDetail(ExtendStatusCode::StorageTransientThrottling).CodeAsString(), "StorageTransientThrottling"); EXPECT_EQ(ExtendStatusDetail(ExtendStatusCode::StorageTransientService).CodeAsString(), "StorageTransientService"); + EXPECT_EQ(ExtendStatusDetail(ExtendStatusCode::LanceWriteContention).CodeAsString(), "LanceWriteContention"); + EXPECT_EQ(ExtendStatusDetail(ExtendStatusCode::LanceResourceNotFound).CodeAsString(), "LanceResourceNotFound"); } // ToString @@ -270,6 +278,8 @@ TEST_F(ExtendStatusTest, ExtendCodesMapToSegcoreErrorCode) { {ExtendStatusCode::StorageTransientService, milvus::StorageTransientError}, {ExtendStatusCode::TxnExhaustedRetry, milvus::StorageError}, {ExtendStatusCode::TxnResolutionFailed, milvus::StorageError}, + {ExtendStatusCode::LanceWriteContention, milvus::StorageTransientError}, + {ExtendStatusCode::LanceResourceNotFound, milvus::ObjectNotExist}, }; for (const auto& test_case : cases) { @@ -304,6 +314,7 @@ TEST_F(ExtendStatusTest, PermanentS3ErrorsAreNotRetriable) { const Case cases[] = { // not-found is fine-grained: ObjectNotExist(2017), still permanent {ExtendStatusCode::AwsErrorNotFound, "AwsErrorNotFound", milvus::ObjectNotExist}, + {ExtendStatusCode::LanceResourceNotFound, "LanceResourceNotFound", milvus::ObjectNotExist}, {ExtendStatusCode::AwsErrorAccessDenied, "AwsErrorAccessDenied", milvus::StorageError}, {ExtendStatusCode::AwsErrorNonRetryable, "AwsErrorNonRetryable", milvus::StorageError}, }; diff --git a/cpp/test/ffi/ffi_filesystem_test.c b/cpp/test/ffi/ffi_filesystem_test.c index 5ac362138..0d3e6adc7 100644 --- a/cpp/test/ffi/ffi_filesystem_test.c +++ b/cpp/test/ffi/ffi_filesystem_test.c @@ -907,6 +907,7 @@ static void test_retryable_errcode_helper(void) { ck_assert(loon_ffi_is_retryable_errcode(loon_errcode_transient_throttling)); ck_assert(loon_ffi_is_retryable_errcode(loon_errcode_transient_service)); ck_assert(loon_ffi_is_retryable_errcode(loon_errcode_aws_no_such_upload)); + ck_assert(loon_ffi_is_retryable_errcode(loon_errcode_lance_write_contention)); ck_assert(!loon_ffi_is_retryable_errcode(loon_errcode_success)); ck_assert(!loon_ffi_is_retryable_errcode(loon_errcode_arrow)); @@ -918,6 +919,7 @@ static void test_retryable_errcode_helper(void) { ck_assert(!loon_ffi_is_retryable_errcode(loon_errcode_aws_non_retryable)); ck_assert(!loon_ffi_is_retryable_errcode(loon_errcode_txn_exhausted_retry)); ck_assert(!loon_ffi_is_retryable_errcode(loon_errcode_txn_resolution_failed)); + ck_assert(!loon_ffi_is_retryable_errcode(loon_errcode_lance_resource_not_found)); ck_assert(!loon_ffi_is_retryable_errcode(99999)); } diff --git a/cpp/test/format/lance/lance_bridge_error_test.cpp b/cpp/test/format/lance/lance_bridge_error_test.cpp index f33b9d6e3..4167b44f2 100644 --- a/cpp/test/format/lance/lance_bridge_error_test.cpp +++ b/cpp/test/format/lance/lance_bridge_error_test.cpp @@ -45,16 +45,34 @@ TEST(BridgeErrorTest, NotFoundCodeBecomesEnoentDetail) { EXPECT_EQ(ToSegcoreError(status).get_error_code(), milvus::ObjectNotExist); } -TEST(BridgeErrorTest, TransientCodeBecomesRetryableExtendDetail) { - auto status = MakeBridgeErrorStatus(std::string(kMarker) + "109; too much write contention"); +TEST(BridgeErrorTest, LanceContentionHasItsOwnRetryableDetail) { + auto status = MakeBridgeErrorStatus(std::string(kMarker) + "113; too much write contention"); ASSERT_TRUE(status.IsIOError()); auto detail = ExtendStatusDetail::UnwrapStatus(status); ASSERT_NE(detail, nullptr); - EXPECT_EQ(detail->code(), ExtendStatusCode::StorageTransientThrottling); + EXPECT_EQ(detail->code(), ExtendStatusCode::LanceWriteContention); EXPECT_TRUE(detail->retryable()); EXPECT_EQ(ToSegcoreError(status).get_error_code(), milvus::StorageTransientError); } +TEST(BridgeErrorTest, ObjectStoreThrottlingKeepsCode109) { + auto status = MakeBridgeErrorStatus(std::string(kMarker) + "109; S3 SlowDown"); + auto detail = ExtendStatusDetail::UnwrapStatus(status); + ASSERT_NE(detail, nullptr); + EXPECT_EQ(detail->code(), ExtendStatusCode::StorageTransientThrottling); + EXPECT_TRUE(detail->retryable()); +} + +TEST(BridgeErrorTest, LanceResourceNotFoundDoesNotBecomeDatasetEnoent) { + auto status = MakeBridgeErrorStatus(std::string(kMarker) + "114; version 7 not found"); + ASSERT_TRUE(status.IsIOError()); + auto detail = ExtendStatusDetail::UnwrapStatus(status); + ASSERT_NE(detail, nullptr); + EXPECT_EQ(detail->code(), ExtendStatusCode::LanceResourceNotFound); + EXPECT_NE(arrow::internal::ErrnoFromStatus(status), ENOENT); + EXPECT_EQ(ToSegcoreError(status).get_error_code(), milvus::ObjectNotExist); +} + TEST(BridgeErrorTest, BridgePrivateCodesMapToArrowStatusCodes) { auto corrupt = MakeBridgeErrorStatus(std::string(kMarker) + "1001; corrupt file"); EXPECT_TRUE(corrupt.IsInvalid()); diff --git a/python/milvus_storage/_ffi.py b/python/milvus_storage/_ffi.py index 955e8320a..17428f2b9 100644 --- a/python/milvus_storage/_ffi.py +++ b/python/milvus_storage/_ffi.py @@ -34,6 +34,8 @@ "loon_errcode_transient_service", "loon_errcode_txn_exhausted_retry", "loon_errcode_txn_resolution_failed", + "loon_errcode_lance_write_contention", + "loon_errcode_lance_resource_not_found", ) # Chunk metadata type flags from ffi_c.h @@ -115,6 +117,8 @@ extern int loon_errcode_transient_service; extern int loon_errcode_txn_exhausted_retry; extern int loon_errcode_txn_resolution_failed; + extern int loon_errcode_lance_write_contention; + extern int loon_errcode_lance_resource_not_found; int loon_ffi_is_success(LoonFFIResult* result); const char* loon_ffi_get_errmsg(LoonFFIResult* result); From 9b6230a327ddaa360dc161a8a987345e371cfca8 Mon Sep 17 00:00:00 2001 From: xiaofanluan Date: Wed, 29 Jul 2026 15:46:52 -0700 Subject: [PATCH 7/9] fix: install protoc for the Rust bridge test step 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) Claude-Session: https://claude.ai/code/session_011HDgMok3nR8ZNugKWWQQK2 Signed-off-by: xiaofanluan --- .github/workflows/cpp-ci.yml | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/.github/workflows/cpp-ci.yml b/.github/workflows/cpp-ci.yml index 94f0dfc15..e5f843c31 100644 --- a/.github/workflows/cpp-ci.yml +++ b/.github/workflows/cpp-ci.yml @@ -29,8 +29,12 @@ jobs: # Conan 2.x CMakeDeps doesn't propagate system_libs through shared library targets, # so libaio (required by folly) must be explicitly installed and linked. + # protobuf-compiler is needed by the Rust bridge test step below: `cargo test` + # uses the dev profile, which rebuilds the dependency tree and runs + # lance-encoding's build script (prost-build wants protoc). The Build step + # above gets protoc from conan, but that environment is not inherited here. - name: Install system libraries - run: sudo apt-get update && sudo apt-get install -y libaio-dev + run: sudo apt-get update && sudo apt-get install -y libaio-dev protobuf-compiler - name: Setup Rust id: rust-toolchain From 3df645b2cdc731b78d650180894d9842cf5caa53 Mon Sep 17 00:00:00 2001 From: xiaofanluan Date: Wed, 29 Jul 2026 17:47:28 -0700 Subject: [PATCH 8/9] fix: unwrap Wrapped/External so batched reads keep their classification 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 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) Claude-Session: https://claude.ai/code/session_011HDgMok3nR8ZNugKWWQQK2 Signed-off-by: xiaofanluan --- .../format/bridge/rust/src/bridge_error.rs | 137 ++++++++++++++---- 1 file changed, 106 insertions(+), 31 deletions(-) diff --git a/cpp/src/format/bridge/rust/src/bridge_error.rs b/cpp/src/format/bridge/rust/src/bridge_error.rs index 47994bec2..f902154a1 100644 --- a/cpp/src/format/bridge/rust/src/bridge_error.rs +++ b/cpp/src/format/bridge/rust/src/bridge_error.rs @@ -127,37 +127,17 @@ pub fn classify_lance_error(e: &LanceError) -> Option { } // IO wraps the underlying object_store error as a boxed source; // downcast to recover the typed variant. - LanceError::IO { source, .. } => match source.downcast_ref::() { - // A missing object while operating inside a dataset is not proof - // that the dataset itself is absent. Preserve ObjectNotExist - // without emitting the writer's create-if-missing ENOENT signal. - Some(object_store::Error::NotFound { .. }) => Some(LOON_LANCE_RESOURCE_NOT_FOUND), - Some( - object_store::Error::PermissionDenied { .. } - | object_store::Error::Unauthenticated { .. }, - ) => Some(LOON_AWS_ERROR_ACCESS_DENIED), - Some(object_store::Error::Precondition { .. }) => { - Some(LOON_AWS_ERROR_PRECONDITION_FAILED) - } - Some( - object_store::Error::NotSupported { .. } - | object_store::Error::NotImplemented { .. }, - ) => Some(BRIDGE_ERRCODE_NOT_SUPPORTED), - // Generic carries the post-retry HTTP failure. The typed carrier - // (client::retry::RetryError, which has .status()) is pub(crate) - // in object_store and cannot be downcast from here, so the status - // code is recovered from the stable Display pattern of - // RequestError::Status ("non-2xx status code: NNN"). Fail-safe by - // construction: if object_store ever rewords it, this returns - // None and the error lands in the conservative non-retriable - // bucket -- it can never mis-tag a permanent error as transient. - Some(object_store::Error::Generic { source, .. }) => { - classify_http_status_in_message(&source.to_string()) - } - // Anything else: no positive transient/permanent signal survives, - // so stay untagged (conservative). - _ => None, - }, + LanceError::IO { source, .. } => classify_boxed_source(source.as_ref()), + // Wrappers that carry a classified error inside. lance-io's batch read + // scheduler stashes the failing task's error and re-wraps it on drop + // (`Error::wrapped(...)` in lance-io/src/scheduler.rs), and the encoding + // decoder does the same, so a plain S3 throttle on any batched read + // arrives here as Wrapped(IO(object_store::Generic)) rather than IO. + // Without unwrapping, a retriable failure would be reported as a + // permanent one -- the classification is already there, just one box + // deeper. Recursion terminates because each step strips one layer. + LanceError::Wrapped { error, .. } => classify_boxed_source(error.as_ref()), + LanceError::External { source } => classify_boxed_source(source.as_ref()), // InvalidInput deliberately NOT tagged as caller input: the strings we // feed lance are mostly assembled by this library itself, so blaming // the caller would misroute retries (see the 2007/2020/2021 @@ -166,6 +146,53 @@ pub fn classify_lance_error(e: &LanceError) -> Option { } } +/// Classify a boxed error carried by `LanceError::IO` / `Wrapped` / `External`. +/// +/// The box can hold either the underlying `object_store::Error` or another +/// `LanceError` that some layer re-wrapped, so try both. Anything else stays +/// untagged (conservative non-retriable). +fn classify_boxed_source( + source: &(dyn std::error::Error + Send + Sync + 'static), +) -> Option { + if let Some(store_error) = source.downcast_ref::() { + return classify_object_store_error(store_error); + } + if let Some(lance_error) = source.downcast_ref::() { + return classify_lance_error(lance_error); + } + None +} + +/// Classify the typed `object_store::Error` that backs lance's IO. +fn classify_object_store_error(error: &object_store::Error) -> Option { + match error { + // A missing object while operating inside a dataset is not proof that + // the dataset itself is absent. Preserve ObjectNotExist without + // emitting the writer's create-if-missing ENOENT signal. + object_store::Error::NotFound { .. } => Some(LOON_LANCE_RESOURCE_NOT_FOUND), + object_store::Error::PermissionDenied { .. } + | object_store::Error::Unauthenticated { .. } => Some(LOON_AWS_ERROR_ACCESS_DENIED), + object_store::Error::Precondition { .. } => Some(LOON_AWS_ERROR_PRECONDITION_FAILED), + object_store::Error::NotSupported { .. } | object_store::Error::NotImplemented { .. } => { + Some(BRIDGE_ERRCODE_NOT_SUPPORTED) + } + // Generic carries the post-retry HTTP failure. The typed carrier + // (client::retry::RetryError, which has .status()) is pub(crate) in + // object_store and cannot be downcast from here, so the status code is + // recovered from the stable Display pattern of RequestError::Status + // ("non-2xx status code: NNN"). Fail-safe by construction: if + // object_store ever rewords it, this returns None and the error lands + // in the conservative non-retriable bucket -- it can never mis-tag a + // permanent error as transient. + object_store::Error::Generic { source, .. } => { + classify_http_status_in_message(&source.to_string()) + } + // Anything else: no positive transient/permanent signal survives, so + // stay untagged (conservative). + _ => None, + } +} + /// Recover the HTTP status from object_store's post-retry error message /// ("Server returned non-2xx status code: NNN: ..."). Only well-known /// transient statuses are tagged; anything else stays untagged. @@ -262,6 +289,54 @@ mod tests { ); } + // lance-io's batch read scheduler stashes the failing task's error and + // re-wraps it when the batch drops (`Error::wrapped(...)`, + // lance-io/src/scheduler.rs), so on any batched read a throttle arrives as + // Wrapped(IO(object_store::Generic)) rather than IO(..). Before unwrapping, + // that turned the single most retriable condition into a permanent error. + #[test] + fn wrapped_errors_keep_the_inner_classification() { + let throttled = LanceError::from(object_store::Error::Generic { + store: "S3", + source: "Server returned non-2xx status code: 503: slow down".into(), + }); + assert_eq!( + classify_lance_error(&throttled), + Some(LOON_TRANSIENT_SERVICE) + ); + + let wrapped = LanceError::wrapped(Box::new(throttled)); + assert_eq!(classify_lance_error(&wrapped), Some(LOON_TRANSIENT_SERVICE)); + + // Nested wrapping keeps working: each step strips one layer. + let twice = LanceError::wrapped(Box::new(LanceError::wrapped(Box::new( + LanceError::from(object_store::Error::NotFound { + path: "a/b".to_string(), + source: "missing".into(), + }), + )))); + assert_eq!( + classify_lance_error(&twice), + Some(LOON_LANCE_RESOURCE_NOT_FOUND) + ); + + // A box holding the object_store error directly is classified too. + let external = LanceError::External { + source: Box::new(object_store::Error::PermissionDenied { + path: "a/b".to_string(), + source: "denied".into(), + }), + }; + assert_eq!( + classify_lance_error(&external), + Some(LOON_AWS_ERROR_ACCESS_DENIED) + ); + + // A wrapper with nothing classifiable inside stays untagged. + let opaque = LanceError::wrapped(Box::new(std::io::Error::other("opaque"))); + assert_eq!(classify_lance_error(&opaque), None); + } + #[test] fn generic_throttle_status_is_tagged_transient() { let e = LanceError::from(object_store::Error::Generic { From 9cf5ddc6f7855b59a1fcdb352dcc7f3838da1818 Mon Sep 17 00:00:00 2001 From: xiaofanluan Date: Wed, 29 Jul 2026 22:07:58 -0700 Subject: [PATCH 9/9] enhance: classify iceberg planning errors 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 #597 (introduces bridge_error.rs); branched from it. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_011HDgMok3nR8ZNugKWWQQK2 Signed-off-by: xiaofanluan --- .../format/bridge/rust/src/bridge_error.rs | 210 ++++++++++++++++++ .../bridge/rust/src/iceberg_bridgeimpl.rs | 19 ++ 2 files changed, 229 insertions(+) diff --git a/cpp/src/format/bridge/rust/src/bridge_error.rs b/cpp/src/format/bridge/rust/src/bridge_error.rs index f902154a1..9f1a168ef 100644 --- a/cpp/src/format/bridge/rust/src/bridge_error.rs +++ b/cpp/src/format/bridge/rust/src/bridge_error.rs @@ -48,6 +48,7 @@ pub const BRIDGE_ERRCODE_MARKER: &str = "__LOON_RUST_BRIDGE_ERRCODE__="; pub const LOON_FILE_NOT_FOUND: i32 = 12; /// Mirror of the ExtendStatusCode transient tags (`ffi_error_code.h` 101-112). pub const LOON_AWS_ERROR_PRECONDITION_FAILED: i32 = 103; +pub const LOON_AWS_ERROR_NOT_FOUND: i32 = 104; pub const LOON_AWS_ERROR_ACCESS_DENIED: i32 = 105; pub const LOON_TRANSIENT_TIMEOUT: i32 = 108; pub const LOON_TRANSIENT_THROTTLING: i32 = 109; @@ -212,6 +213,100 @@ fn classify_http_status_in_message(msg: &str) -> Option { } } +/// Classify an `opendal::Error`. This is iceberg's IO layer (via +/// iceberg-storage-opendal), the counterpart of `object_store` under lance. +/// +/// opendal carries the producer's own retriability verdict in +/// `is_temporary()`, so unlike the object_store path we do not have to recover +/// anything from prose. +pub fn classify_opendal_error(e: &opendal::Error) -> Option { + use opendal::ErrorKind; + match e.kind() { + ErrorKind::NotFound => Some(LOON_AWS_ERROR_NOT_FOUND), + ErrorKind::PermissionDenied => Some(LOON_AWS_ERROR_ACCESS_DENIED), + ErrorKind::ConditionNotMatch => Some(LOON_AWS_ERROR_PRECONDITION_FAILED), + ErrorKind::RateLimited => Some(LOON_TRANSIENT_THROTTLING), + ErrorKind::Unsupported => Some(BRIDGE_ERRCODE_NOT_SUPPORTED), + // ConfigInvalid is the caller's/operator's mistake, not ours and not + // the store's. There is no user-error code on this channel yet, so it + // stays untagged rather than being mislabelled as a storage failure. + // Revisit once the user/system axis lands. + ErrorKind::ConfigInvalid => None, + // No specific kind, but opendal itself says the condition may clear. + // Taking the producer's verdict rather than inventing one. + _ if e.is_temporary() => Some(LOON_TRANSIENT_SERVICE), + _ => None, + } +} + +/// Classify an `iceberg::Error`. +/// +/// Scope note: iceberg's Rust side only *plans* (`iceberg_plan_files`); the +/// data files it returns are read by the C++ parquet reader on the C++ +/// filesystem, which already classifies. So this covers metadata/manifest +/// access and snapshot resolution, not the read path. +pub fn classify_iceberg_error(e: &iceberg::Error) -> Option { + use iceberg::ErrorKind; + match e.kind() { + // The table or namespace the caller pointed at does not exist. + ErrorKind::TableNotFound | ErrorKind::NamespaceNotFound => Some(LOON_AWS_ERROR_NOT_FOUND), + ErrorKind::PreconditionFailed => Some(LOON_AWS_ERROR_PRECONDITION_FAILED), + ErrorKind::FeatureUnsupported => Some(BRIDGE_ERRCODE_NOT_SUPPORTED), + // Malformed table metadata / manifest: re-reading the same bytes gives + // the same result. + ErrorKind::DataInvalid => Some(BRIDGE_ERRCODE_DATA_CORRUPT), + // Write-path conditions. plan_files is read-only so these should not + // occur; leaving them untagged avoids diluting the CAS-specific + // conflict code with a second meaning. + ErrorKind::TableAlreadyExists + | ErrorKind::NamespaceAlreadyExists + | ErrorKind::CatalogCommitConflicts => None, + // Unexpected is iceberg's catch-all: the real signal, if any, is the + // opendal error further down the chain, which the anyhow walk below + // recovers. + _ => None, + } +} + +/// Classify an error that reached the bridge boundary as `anyhow::Error`. +/// +/// anyhow keeps the concrete types, so walking the chain recovers the typed +/// error a `?` erased. First positive identification wins; everything else +/// stays untagged and lands in the conservative non-retriable bucket. +pub fn classify_anyhow_error(e: &anyhow::Error) -> Option { + for cause in e.chain() { + if let Some(iceberg_error) = cause.downcast_ref::() { + if let Some(code) = classify_iceberg_error(iceberg_error) { + return Some(code); + } + } + if let Some(opendal_error) = cause.downcast_ref::() { + if let Some(code) = classify_opendal_error(opendal_error) { + return Some(code); + } + } + } + None +} + +impl From for BridgeError { + fn from(e: iceberg::Error) -> Self { + BridgeError { + code: classify_iceberg_error(&e), + msg: e.to_string(), + } + } +} + +impl From for BridgeError { + fn from(e: anyhow::Error) -> Self { + BridgeError { + code: classify_anyhow_error(&e), + msg: format!("{e:#}"), + } + } +} + impl From for BridgeError { fn from(e: LanceError) -> Self { BridgeError { @@ -294,6 +389,121 @@ mod tests { // lance-io/src/scheduler.rs), so on any batched read a throttle arrives as // Wrapped(IO(object_store::Generic)) rather than IO(..). Before unwrapping, // that turned the single most retriable condition into a permanent error. + // Version pin for the iceberg IO path. + // + // iceberg-storage-opendal turns every IO failure into + // Error::new(ErrorKind::Unexpected, "Failure in doing io operation") + // .with_source(opendal_error) + // (see its utils::from_opendal_error). So the iceberg kind carries no IO + // signal at all -- the only signal is the typed opendal error in the + // source chain, and recovering it depends on THIS crate's opendal being + // the same version iceberg-storage-opendal links. A version skew makes the + // downcast return None silently and every iceberg IO failure would fall + // into the untagged bucket. + // + // This reproduces that exact shape, so a skew fails the test instead of + // quietly degrading. A compile-time pin is not possible here: + // `with_source` accepts any error type, so it would not constrain the + // version. + #[test] + fn opendal_cause_is_recoverable_through_the_iceberg_wrapper() { + let as_storage_opendal_builds_it = iceberg::Error::new( + iceberg::ErrorKind::Unexpected, + "Failure in doing io operation", + ) + .with_source(opendal::Error::new( + opendal::ErrorKind::RateLimited, + "slow down", + )); + + // The iceberg kind alone yields nothing -- everything is Unexpected. + assert_eq!(classify_iceberg_error(&as_storage_opendal_builds_it), None); + + // The chain walk is what recovers it. + let wrapped = anyhow::Error::from(as_storage_opendal_builds_it).context("plan files"); + assert_eq!(classify_anyhow_error(&wrapped), Some(LOON_TRANSIENT_THROTTLING)); + } + + #[test] + fn iceberg_kinds_are_classified() { + use iceberg::ErrorKind; + let cases = [ + (ErrorKind::TableNotFound, Some(LOON_AWS_ERROR_NOT_FOUND)), + (ErrorKind::NamespaceNotFound, Some(LOON_AWS_ERROR_NOT_FOUND)), + (ErrorKind::PreconditionFailed, Some(LOON_AWS_ERROR_PRECONDITION_FAILED)), + (ErrorKind::FeatureUnsupported, Some(BRIDGE_ERRCODE_NOT_SUPPORTED)), + (ErrorKind::DataInvalid, Some(BRIDGE_ERRCODE_DATA_CORRUPT)), + // Write-path conflicts stay untagged rather than diluting the + // CAS-specific conflict code; plan_files is read-only anyway. + (ErrorKind::TableAlreadyExists, None), + (ErrorKind::CatalogCommitConflicts, None), + // Catch-all: the signal, if any, lives in the opendal cause. + (ErrorKind::Unexpected, None), + ]; + for (kind, expected) in cases { + let e = iceberg::Error::new(kind, "boom"); + assert_eq!(classify_iceberg_error(&e), expected, "{kind:?}"); + } + } + + #[test] + fn opendal_kinds_are_classified() { + use opendal::ErrorKind as OdKind; + let cases = [ + (OdKind::NotFound, Some(LOON_AWS_ERROR_NOT_FOUND)), + (OdKind::PermissionDenied, Some(LOON_AWS_ERROR_ACCESS_DENIED)), + (OdKind::ConditionNotMatch, Some(LOON_AWS_ERROR_PRECONDITION_FAILED)), + (OdKind::RateLimited, Some(LOON_TRANSIENT_THROTTLING)), + (OdKind::Unsupported, Some(BRIDGE_ERRCODE_NOT_SUPPORTED)), + // Caller/operator mistake; no user-error code on this channel yet. + (OdKind::ConfigInvalid, None), + (OdKind::Unexpected, None), + ]; + for (kind, expected) in cases { + let e = opendal::Error::new(kind, "boom"); + assert_eq!(classify_opendal_error(&e), expected, "{kind:?}"); + } + + // opendal's own retriability bit is honoured when no specific kind + // applies -- taking the producer's verdict rather than inventing one. + let temporary = opendal::Error::new(OdKind::Unexpected, "flaky").set_temporary(); + assert_eq!(classify_opendal_error(&temporary), Some(LOON_TRANSIENT_SERVICE)); + } + + // The point of the anyhow walk: `?` erases the concrete type into + // anyhow::Error, and every iceberg entry point does that several times + // over. Without walking the chain the classification is lost. + #[test] + fn anyhow_chain_recovers_the_typed_cause() { + let throttled: anyhow::Error = + opendal::Error::new(opendal::ErrorKind::RateLimited, "slow down").into(); + let wrapped = throttled.context("load table metadata"); + assert_eq!(classify_anyhow_error(&wrapped), Some(LOON_TRANSIENT_THROTTLING)); + + let missing: anyhow::Error = + iceberg::Error::new(iceberg::ErrorKind::TableNotFound, "no table").into(); + assert_eq!( + classify_anyhow_error(&missing.context("plan files")), + Some(LOON_AWS_ERROR_NOT_FOUND) + ); + + // Nothing classifiable in the chain stays untagged. + let opaque = anyhow::anyhow!("metadata_location must not be empty"); + assert_eq!(classify_anyhow_error(&opaque), None); + + // And the BridgeError conversion carries the code plus a full + // `{:#}` chain rendering, so context is not lost. + let converted = BridgeError::from( + anyhow::Error::from(opendal::Error::new( + opendal::ErrorKind::PermissionDenied, + "denied", + )) + .context("open manifest"), + ); + assert_eq!(converted.code, Some(LOON_AWS_ERROR_ACCESS_DENIED)); + assert!(converted.msg.contains("open manifest"), "{}", converted.msg); + } + #[test] fn wrapped_errors_keep_the_inner_classification() { let throttled = LanceError::from(object_store::Error::Generic { diff --git a/cpp/src/format/bridge/rust/src/iceberg_bridgeimpl.rs b/cpp/src/format/bridge/rust/src/iceberg_bridgeimpl.rs index ac473f553..48ec8460c 100644 --- a/cpp/src/format/bridge/rust/src/iceberg_bridgeimpl.rs +++ b/cpp/src/format/bridge/rust/src/iceberg_bridgeimpl.rs @@ -246,11 +246,30 @@ fn build_delete_metadata(task: &FileScanTask) -> Vec { .collect() } +/// cxx entry point. Classification happens here and only here: the body below +/// keeps using `anyhow` so `?` stays ergonomic, and `BridgeError::from` walks +/// the anyhow chain once at the boundary to recover the typed +/// `iceberg::Error` / `opendal::Error` that `?` erased. pub fn iceberg_plan_files( metadata_location: &str, snapshot_id: i64, storage_options_keys: Vec, storage_options_values: Vec, +) -> crate::bridge_error::BridgeResult> { + iceberg_plan_files_impl( + metadata_location, + snapshot_id, + storage_options_keys, + storage_options_values, + ) + .map_err(crate::bridge_error::BridgeError::from) +} + +fn iceberg_plan_files_impl( + metadata_location: &str, + snapshot_id: i64, + storage_options_keys: Vec, + storage_options_values: Vec, ) -> Result, anyhow::Error> { if metadata_location.is_empty() { anyhow::bail!("metadata_location must not be empty");