SteNicholas commented on code in PR #392:
URL: https://github.com/apache/paimon-cpp/pull/392#discussion_r4131837104
##########
src/paimon/format/blob/blob_format_writer.cpp:
##########
@@ -262,38 +355,68 @@ Result<std::unique_ptr<InputStream>>
BlobFormatWriter::OpenDescriptorInputStream
if (write_null_on_missing_file_) {
Result<bool> exists = fs_->Exists(blob->Uri());
if (exists.ok() && !exists.value()) {
- return HandleMissingFile(blob->Uri());
+ return HandleMissingFile(blob->Uri(), element_index);
}
}
- return HandleFetchFailure(blob->Uri(), opened.status());
+ return HandleFetchFailure(blob->Uri(), element_index, opened.status());
}
return std::move(opened).value();
}
-std::unique_ptr<InputStream> BlobFormatWriter::HandleMissingFile(const
std::string& blob_uri) {
- PAIMON_LOG_WARN(logger_, "Blob file %s does not exist, writing NULL for
BLOB field %s into %s",
- blob_uri.c_str(), blob_field_name_.c_str(), uri_.c_str());
+std::unique_ptr<InputStream> BlobFormatWriter::HandleMissingFile(
+ const std::string& blob_uri, std::optional<int32_t> element_index) {
+ PAIMON_LOG_WARN(logger_, "Blob file %s does not exist, writing NULL for
%s", blob_uri.c_str(),
+ DescribeValue(element_index).c_str());
++null_on_missing_file_count_;
return std::unique_ptr<InputStream>();
}
Result<std::unique_ptr<InputStream>> BlobFormatWriter::HandleFetchFailure(
- const std::string& blob_uri, const Status& status) {
+ const std::string& blob_uri, std::optional<int32_t> element_index, const
Status& status) {
if (!write_null_on_fetch_failure_) {
- return status;
+ return AddFailureContext(status, fmt::format("failed to fetch blob {}
for", blob_uri),
+ element_index);
}
- PAIMON_LOG_WARN(logger_, "Failed to fetch blob %s, writing NULL for BLOB
field %s into %s: %s",
- blob_uri.c_str(), blob_field_name_.c_str(), uri_.c_str(),
- status.ToString().c_str());
+ PAIMON_LOG_WARN(logger_, "Failed to fetch blob %s, writing NULL for %s:
%s", blob_uri.c_str(),
+ DescribeValue(element_index).c_str(),
status.ToString().c_str());
++null_on_fetch_failure_count_;
return std::unique_ptr<InputStream>();
}
+Status BlobFormatWriter::AddFailureContext(const Status& status, const
std::string& action,
+ std::optional<int32_t>
element_index) const {
+ return status.WithMessage(action, " ", DescribeValue(element_index), ": ",
status.message());
+}
+
+std::string BlobFormatWriter::DescribeValue(std::optional<int32_t>
element_index) const {
+ // The entry of the row being written is appended to bin_lengths_ once the
row is done.
+ const size_t row = bin_lengths_.size();
+ if (!element_index) {
+ return fmt::format("BLOB field {} in row {} of blob file {}",
blob_field_name_, row, uri_);
+ }
+ return fmt::format("element {} of ARRAY<BLOB> field {} in row {} of blob
file {}",
+ *element_index, blob_field_name_, row, uri_);
+}
+
+Result<int32_t> BlobFormatWriter::ToIndexLength(size_t index_size) const {
+ if (index_size > static_cast<size_t>(std::numeric_limits<int32_t>::max()))
{
+ return Status::Invalid(
+ fmt::format("compressed index too large: {} bytes in blob file
{}", index_size, uri_));
+ }
+ return static_cast<int32_t>(index_size);
+}
+
Status BlobFormatWriter::WriteBytes(const char* data, int64_t length) {
- PAIMON_ASSIGN_OR_RAISE(int64_t actual, out_->Write(data, length));
- if (actual != length) {
+ Result<int64_t> actual = out_->Write(data, length);
+ if (!actual.ok()) {
+ const Status& status = actual.status();
+ return status.WithMessage("failed to write blob file ", uri_, ": ",
status.message());
+ }
+ if (actual.value() != length) {
return Status::Invalid(
- fmt::format("unexpected actual length {} not match with expect
{}", actual, length));
+ fmt::format("failed to write blob file {}: unexpected actual
length {} not match with "
+ "expect {}",
+ uri_, actual.value(), length));
}
return Status::OK();
}
Review Comment:
Aligned. As `ReusingBlobRefStreamProvider#openBounded` does, the stream of a
descriptor with a known length is now bounded by its range rather than by the
file, so a range that ends past the end of the file, even one that starts
beyond it, fails during the copy and fails the write even with
`blob-write-null-on-fetch-failure` enabled.
`TestArrayBlobWriteNullOnUnreachableElements` checks this for
`bad_length_descriptor`, and `TestDataEndingEarlyFailsWrite` for a `BLOB`
value. An offset past the end of the file remains a fetch failure only for a
descriptor with a dynamic length, which C++ cannot open; this difference from
Java is documented in `write.rst`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
