Skip to content

Commit 0ed29b2

Browse files
SteNicholasclaude
andcommitted
fix(blob): identify a missing blob file with FileSystem::Exists
BlobFormatWriter told a missing file apart from a fetch failure by testing Status::NotExist on a failed descriptor open. That makes the two write-null options depend on every file system plugin agreeing on which status code a missing file maps to, which is hard to keep uniform across all plugin interfaces: supporting it already required mapping JDO_FILE_NOT_FOUND_ERROR to Status::NotExist in the jindo error macro, and any plugin that reports a missing file as a plain IO error silently disables 'blob-write-null-on-missing-file'. Decide it with FileSystem::Exists instead. Java classifies the same way, through UriReader.FileUriReader.exists -> FileIO.exists; the only error code its writer tests is HTTP 404, which is protocol-defined rather than plugin-specific. The check runs only when the two options treat the two cases differently, so the default path pays nothing for it. Handle the two failures that were reachable but unclassified: - A failed existence check leaves it unknown whether the file is there, so it is reported as a fetch failure rather than assuming either answer. - A descriptor that cannot be deserialized is a fetch failure too, which is what BLOB_WRITE_NULL_ON_FETCH_FAILURE already documented but the code did not do; its status keeps its original code and gains an "invalid blob descriptor" prefix so the cause is named. Create() now rejects a null file system, so that configuration error surfaces at construction instead of being silently written out as NULL rows. Report both conversions as blob.write.null-on-missing-file.count and blob.write.null-on-fetch-failure.count, and name the blob URI, the BLOB field and the output file in the write-null warnings. Also drop the ${CMAKE_DL_LIBS} entries listed by hand on targets that never call dlopen/dlsym/dladdr themselves -- a grep over src/ and include/ finds no such call. Those entries were carried on behalf of arrow, which declares dl as a usage requirement on its own interface, so CMake places -ldl after libarrow.a automatically on linkers that resolve symbols strictly left-to-right, and every target that dropped an entry links arrow and inherits it transitively. dl is now declared only on the real providers (arrow, lucene, jindosdk::nextarch), with the convention recorded next to arrow's declaration so consumers do not add it back. Follow-up to the review comment on alibaba#421. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent a4c1fef commit 0ed29b2

15 files changed

Lines changed: 258 additions & 51 deletions

File tree

benchmark/CMakeLists.txt

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,6 @@ if(PAIMON_BUILD_BENCHMARKS)
5757
${PAIMON_BENCHMARK_STATIC_LINK_LIBS}
5858
test_utils_static
5959
Threads::Threads
60-
${CMAKE_DL_LIBS}
6160
${PAIMON_BENCHMARK_PLATFORM_LINK_LIBS}
6261
${PAIMON_BENCHMARK_LINK_TOOLCHAIN}
6362
EXTRA_INCLUDES

cmake_modules/ThirdpartyToolchain.cmake

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1638,6 +1638,8 @@ macro(build_arrow)
16381638

16391639
# libarrow.a calls dlsym; keep ${CMAKE_DL_LIBS} in the interface so -ldl is placed
16401640
# after libarrow.a on linkers that resolve symbols strictly left-to-right.
1641+
# This is the single source of truth for dl: targets linking arrow inherit it
1642+
# transitively and must not list ${CMAKE_DL_LIBS} again themselves.
16411643
target_link_libraries(arrow
16421644
INTERFACE zstd
16431645
snappy

src/paimon/CMakeLists.txt

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -392,7 +392,6 @@ add_paimon_lib(paimon
392392
arrow
393393
tbb
394394
glog
395-
${CMAKE_DL_LIBS}
396395
fmt
397396
roaring_bitmap
398397
xxhash

src/paimon/common/file_index/CMakeLists.txt

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,6 @@ add_paimon_lib(paimon_file_index
4242
arrow
4343
fmt
4444
xxhash
45-
${CMAKE_DL_LIBS}
4645
Threads::Threads
4746
SHARED_LINK_LIBS
4847
paimon_shared

src/paimon/common/global_index/CMakeLists.txt

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,6 @@ add_paimon_lib(paimon_global_index
3838
STATIC_LINK_LIBS
3939
arrow
4040
fmt
41-
${CMAKE_DL_LIBS}
4241
Threads::Threads
4342
SHARED_LINK_LIBS
4443
paimon_shared

src/paimon/format/avro/CMakeLists.txt

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,6 @@ if(PAIMON_ENABLE_AVRO)
3838
fmt
3939
avro
4040
tbb
41-
${CMAKE_DL_LIBS}
4241
Threads::Threads
4342
SHARED_LINK_LIBS
4443
paimon_shared

src/paimon/format/blob/CMakeLists.txt

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,6 @@ add_paimon_lib(paimon_blob_file_format
2525
STATIC_LINK_LIBS
2626
arrow
2727
fmt
28-
${CMAKE_DL_LIBS}
2928
Threads::Threads
3029
SHARED_LINK_LIBS
3130
paimon_shared

src/paimon/format/blob/blob_format_writer.cpp

Lines changed: 71 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@
3030
#include "paimon/common/utils/arrow/status_utils.h"
3131
#include "paimon/common/utils/delta_varint_compressor.h"
3232
#include "paimon/data/blob.h"
33+
#include "paimon/fs/file_system.h"
3334
#include "paimon/io/byte_array_input_stream.h"
3435
#include "paimon/logging.h"
3536

@@ -48,6 +49,8 @@ BlobFormatWriter::BlobFormatWriter(const std::shared_ptr<OutputStream>& out, con
4849
pool_(pool),
4950
write_null_on_missing_file_(write_null_on_missing_file),
5051
write_null_on_fetch_failure_(write_null_on_fetch_failure) {
52+
// Create() has already checked that data_type has exactly one BLOB field.
53+
blob_field_name_ = data_type_->field(0)->name();
5154
metrics_ = std::make_shared<MetricsImpl>();
5255
tmp_buffer_ = Bytes::AllocateBytes(kTmpBufferSize, pool_.get());
5356
magic_number_bytes_ = IntegerToLittleEndian<int32_t>(BlobDefs::kMagicNumber, pool_);
@@ -67,6 +70,9 @@ Result<std::unique_ptr<BlobFormatWriter>> BlobFormatWriter::Create(
6770
if (pool == nullptr) {
6871
return Status::Invalid("blob format writer create failed. pool is nullptr");
6972
}
73+
if (fs == nullptr) {
74+
return Status::Invalid("blob format writer create failed. fs is nullptr");
75+
}
7076
if (data_type->num_fields() != 1) {
7177
return Status::Invalid(
7278
fmt::format("blob data type field number {} is not 1", data_type->num_fields()));
@@ -117,6 +123,10 @@ Status BlobFormatWriter::AddBatch(ArrowArray* batch) {
117123
}
118124

119125
Status BlobFormatWriter::Flush() {
126+
metrics_->SetCounter(BlobMetrics::WRITE_NULL_ON_MISSING_FILE_COUNT,
127+
null_on_missing_file_count_);
128+
metrics_->SetCounter(BlobMetrics::WRITE_NULL_ON_FETCH_FAILURE_COUNT,
129+
null_on_fetch_failure_count_);
120130
return out_->Flush();
121131
}
122132

@@ -147,22 +157,14 @@ Status BlobFormatWriter::WriteBlob(std::string_view blob_data) {
147157
PAIMON_ASSIGN_OR_RAISE(bool is_descriptor,
148158
BlobDescriptor::IsBlobDescriptor(blob_data.data(), blob_data.size()));
149159
if (is_descriptor) {
150-
Result<std::unique_ptr<InputStream>> opened = OpenDescriptorInputStream(blob_data);
151-
if (!opened.ok()) {
152-
const Status& status = opened.status();
153-
// A missing file is only handled by 'blob-write-null-on-missing-file'; other fetch
154-
// failures are only handled by 'blob-write-null-on-fetch-failure' (aligned with Java).
155-
bool write_null =
156-
status.IsNotExist() ? write_null_on_missing_file_ : write_null_on_fetch_failure_;
157-
if (write_null) {
158-
PAIMON_LOG_WARN(logger_, "Failed to open blob, writing NULL for BLOB field: %s",
159-
status.ToString().c_str());
160-
bin_lengths_.push_back(BlobDefs::kNullBinLength);
161-
return Status::OK();
162-
}
163-
return status;
160+
PAIMON_ASSIGN_OR_RAISE(std::unique_ptr<InputStream> descriptor_in,
161+
OpenDescriptorInputStream(blob_data));
162+
// A null stream means a write-null option already converted the failure.
163+
if (descriptor_in == nullptr) {
164+
bin_lengths_.push_back(BlobDefs::kNullBinLength);
165+
return Status::OK();
164166
}
165-
in = std::move(opened).value();
167+
in = std::move(descriptor_in);
166168
} else {
167169
in = std::make_unique<ByteArrayInputStream>(blob_data.data(), blob_data.size());
168170
}
@@ -206,10 +208,60 @@ Status BlobFormatWriter::WriteBlob(std::string_view blob_data) {
206208
}
207209

208210
Result<std::unique_ptr<InputStream>> BlobFormatWriter::OpenDescriptorInputStream(
209-
std::string_view blob_data) const {
210-
PAIMON_ASSIGN_OR_RAISE(std::unique_ptr<Blob> blob,
211-
Blob::FromDescriptor(blob_data.data(), blob_data.size()));
212-
return blob->NewInputStream(fs_);
211+
std::string_view blob_data) {
212+
// A descriptor that cannot be deserialized is a fetch failure: the referenced data cannot be
213+
// reached. Its URI is inside the unreadable bytes, hence the placeholder; the underlying
214+
// status comes from the byte reader and never mentions blobs, hence the added context.
215+
Result<std::unique_ptr<Blob>> blob_result =
216+
Blob::FromDescriptor(blob_data.data(), blob_data.size());
217+
if (!blob_result.ok()) {
218+
const Status& status = blob_result.status();
219+
return HandleFetchFailure(
220+
"<unknown>", status.WithMessage("invalid blob descriptor: ", status.message()));
221+
}
222+
std::unique_ptr<Blob> blob = std::move(blob_result).value();
223+
224+
// A missing file is identified by FileSystem::Exists rather than by the status of a failed
225+
// open: file system implementations disagree on which status a missing file maps to. Java
226+
// classifies the same way and also checks up front, which costs one extra call per row whose
227+
// file is present; deferring the check until an open fails would avoid that, but the up-front
228+
// structure is kept deliberately. Skipped when neither option is enabled, since the two cases
229+
// then get the same treatment anyway.
230+
if (write_null_on_missing_file_ || write_null_on_fetch_failure_) {
231+
Result<bool> exists = fs_->Exists(blob->Uri());
232+
if (!exists.ok()) {
233+
// Unknown whether the file is there, so do not assume either answer.
234+
return HandleFetchFailure(blob->Uri(), exists.status());
235+
}
236+
if (!exists.value()) {
237+
if (!write_null_on_missing_file_) {
238+
return Status::NotExist(fmt::format("File '{}' does not exist", blob->Uri()));
239+
}
240+
PAIMON_LOG_WARN(logger_,
241+
"Blob file %s does not exist, writing NULL for BLOB field %s into %s",
242+
blob->Uri().c_str(), blob_field_name_.c_str(), uri_.c_str());
243+
++null_on_missing_file_count_;
244+
return std::unique_ptr<InputStream>();
245+
}
246+
}
247+
248+
Result<std::unique_ptr<InputStream>> opened = blob->NewInputStream(fs_);
249+
if (!opened.ok()) {
250+
return HandleFetchFailure(blob->Uri(), opened.status());
251+
}
252+
return std::move(opened).value();
253+
}
254+
255+
Result<std::unique_ptr<InputStream>> BlobFormatWriter::HandleFetchFailure(
256+
const std::string& blob_uri, const Status& status) {
257+
if (!write_null_on_fetch_failure_) {
258+
return status;
259+
}
260+
PAIMON_LOG_WARN(logger_, "Failed to fetch blob %s, writing NULL for BLOB field %s into %s: %s",
261+
blob_uri.c_str(), blob_field_name_.c_str(), uri_.c_str(),
262+
status.ToString().c_str());
263+
++null_on_fetch_failure_count_;
264+
return std::unique_ptr<InputStream>();
213265
}
214266

215267
Status BlobFormatWriter::WriteBytes(const char* data, int64_t length) {

src/paimon/format/blob/blob_format_writer.h

Lines changed: 28 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -47,14 +47,25 @@ class OutputStream;
4747

4848
namespace paimon::blob {
4949

50+
class BlobMetrics {
51+
public:
52+
/// Number of rows written as NULL because their referenced file did not exist.
53+
static inline const char WRITE_NULL_ON_MISSING_FILE_COUNT[] =
54+
"blob.write.null-on-missing-file.count";
55+
/// Number of rows written as NULL because their referenced data could not be reached.
56+
static inline const char WRITE_NULL_ON_FETCH_FAILURE_COUNT[] =
57+
"blob.write.null-on-fetch-failure.count";
58+
};
59+
5060
// Blob format:
5161
// https://cwiki.apache.org/confluence/display/PAIMON/PIP-35%3A+Introduce+Blob+to+store+multimodal+data
5262
class BlobFormatWriter : public FormatWriter {
5363
public:
54-
/// When opening a descriptor input fails, `write_null_on_missing_file` converts a
55-
/// missing file (Status::NotExist) to a NULL element and `write_null_on_fetch_failure`
56-
/// converts any other open failure; failures during the streaming copy always fail the
57-
/// write. See Options::BLOB_WRITE_NULL_ON_MISSING_FILE / BLOB_WRITE_NULL_ON_FETCH_FAILURE.
64+
/// `write_null_on_missing_file` converts a descriptor whose referenced file does not exist
65+
/// (as reported by FileSystem::Exists) to a NULL element, and `write_null_on_fetch_failure`
66+
/// converts any other failure to access the referenced data; failures during the streaming
67+
/// copy always fail the write.
68+
/// See Options::BLOB_WRITE_NULL_ON_MISSING_FILE / BLOB_WRITE_NULL_ON_FETCH_FAILURE.
5869
static Result<std::unique_ptr<BlobFormatWriter>> Create(
5970
const std::shared_ptr<OutputStream>& out, const std::shared_ptr<arrow::DataType>& data_type,
6071
bool write_null_on_missing_file, bool write_null_on_fetch_failure,
@@ -84,8 +95,15 @@ class BlobFormatWriter : public FormatWriter {
8495
Status WriteBlob(std::string_view blob_data);
8596

8697
/// Deserialize the descriptor and open an input stream on the referenced data.
87-
Result<std::unique_ptr<InputStream>> OpenDescriptorInputStream(
88-
std::string_view blob_data) const;
98+
/// Returns a null stream when the failure is converted to a NULL element by
99+
/// `write_null_on_missing_file_` or `write_null_on_fetch_failure_`.
100+
Result<std::unique_ptr<InputStream>> OpenDescriptorInputStream(std::string_view blob_data);
101+
102+
/// Apply `write_null_on_fetch_failure_` to a failure to reach the referenced data: returns
103+
/// `status` when the option is disabled, and a null stream when it converts the failure to a
104+
/// NULL element. `blob_uri` is only used for logging.
105+
Result<std::unique_ptr<InputStream>> HandleFetchFailure(const std::string& blob_uri,
106+
const Status& status);
89107

90108
Status WriteBytes(const char* data, int64_t length);
91109
Status WriteWithCrc32(const char* data, int64_t length);
@@ -100,15 +118,19 @@ class BlobFormatWriter : public FormatWriter {
100118
uint32_t crc32_ = 0;
101119
std::vector<int64_t> bin_lengths_;
102120
std::shared_ptr<OutputStream> out_;
121+
/// Path of the blob file being written, not of any referenced blob.
103122
std::string uri_;
104123
PAIMON_UNIQUE_PTR<Bytes> tmp_buffer_;
105124
PAIMON_UNIQUE_PTR<Bytes> magic_number_bytes_;
106125
std::shared_ptr<arrow::DataType> data_type_;
126+
std::string blob_field_name_;
107127
std::shared_ptr<FileSystem> fs_;
108128
std::shared_ptr<MemoryPool> pool_;
109129
std::shared_ptr<Metrics> metrics_;
110130
bool write_null_on_missing_file_ = false;
111131
bool write_null_on_fetch_failure_ = false;
132+
uint64_t null_on_missing_file_count_ = 0;
133+
uint64_t null_on_fetch_failure_count_ = 0;
112134
std::unique_ptr<Logger> logger_;
113135
};
114136

0 commit comments

Comments
 (0)