Skip to content

Commit 0bc4270

Browse files
SteNicholasclaude
andcommitted
fix(blob): address review comments on write-null options
- Reorder the write-null parameters before fs/pool in BlobFormatWriter to match the Java constructor order, and document them on Create(). - Map JDO_FILE_NOT_FOUND_ERROR to Status::NotExist directly in PAIMON_RETURN_NOT_OK_FROM_JINDO and drop the _WITH_NOT_EXIST variant, making all jindo call sites consistent with LocalFileSystem. - Enable jindo_utils_test in fs_test when PAIMON_ENABLE_JINDO is ON; it only checks in-memory status conversion and needs no OSS cluster. - Add end-to-end write-null tests in blob_table_inte_test.cpp covering the NULL element merge with data files and the failure paths. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 parent 7b36b8f commit 0bc4270

10 files changed

Lines changed: 254 additions & 130 deletions

src/paimon/CMakeLists.txt

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -791,19 +791,29 @@ if(PAIMON_BUILD_TESTS)
791791
${TEST_STATIC_LINK_LIBS}
792792
${GTEST_LINK_TOOLCHAIN})
793793

794+
# jindo_utils_test only checks the in-memory status conversion and does not need an
795+
# OSS cluster, so it runs whenever jindo is built. The other jindo tests need real
796+
# OSS access and stay disabled.
797+
set(FS_TEST_JINDO_SOURCES)
798+
if(PAIMON_ENABLE_JINDO)
799+
list(APPEND FS_TEST_JINDO_SOURCES fs/jindo/jindo_utils_test.cpp)
800+
endif()
801+
794802
add_paimon_test(fs_test
795803
SOURCES
796804
common/fs/file_system_test.cpp
797805
common/fs/resolving_file_system_test.cpp
798806
fs/local/local_file_test.cpp
807+
${FS_TEST_JINDO_SOURCES}
799808
# fs/jindo/jindo_file_system_factory_test.cpp
800809
# fs/jindo/jindo_file_system_test.cpp
801-
# fs/jindo/jindo_utils_test.cpp
802810
STATIC_LINK_LIBS
803811
paimon_shared
804812
${PAIMON_LOCAL_FILE_SYSTEM_STATIC_LINK_LIBS}
805-
# ${PAIMON_JINDO_FILE_SYSTEM_STATIC_LINK_LIBS}
813+
${PAIMON_JINDO_FILE_SYSTEM_STATIC_LINK_LIBS}
806814
test_utils_static
807-
${GTEST_LINK_TOOLCHAIN})
815+
${GTEST_LINK_TOOLCHAIN}
816+
EXTRA_INCLUDES
817+
${JINDOSDK_INCLUDE_DIR})
808818

809819
endif()

src/paimon/format/blob/blob_file_batch_reader_test.cpp

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -234,8 +234,10 @@ TEST_P(BlobFileBatchReaderTest, EmptyFile) {
234234
file_system->Create(dir->Str() + "/file.blob", /*overwrite=*/true));
235235
std::shared_ptr<arrow::Field> blob_field = BlobUtils::ToArrowField("blob_col");
236236
auto struct_type = arrow::struct_({blob_field});
237-
ASSERT_OK_AND_ASSIGN(std::shared_ptr<BlobFormatWriter> writer,
238-
BlobFormatWriter::Create(output_stream, struct_type, file_system, pool_));
237+
ASSERT_OK_AND_ASSIGN(
238+
std::shared_ptr<BlobFormatWriter> writer,
239+
BlobFormatWriter::Create(output_stream, struct_type, /*write_null_on_missing_file=*/false,
240+
/*write_null_on_fetch_failure=*/false, file_system, pool_));
239241

240242
ASSERT_OK(writer->Flush());
241243
ASSERT_OK(writer->Finish());

src/paimon/format/blob/blob_format_writer.cpp

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -37,10 +37,10 @@ namespace paimon::blob {
3737

3838
BlobFormatWriter::BlobFormatWriter(const std::shared_ptr<OutputStream>& out, const std::string& uri,
3939
const std::shared_ptr<arrow::DataType>& data_type,
40-
const std::shared_ptr<FileSystem>& fs,
41-
const std::shared_ptr<MemoryPool>& pool,
4240
bool write_null_on_missing_file,
43-
bool write_null_on_fetch_failure)
41+
bool write_null_on_fetch_failure,
42+
const std::shared_ptr<FileSystem>& fs,
43+
const std::shared_ptr<MemoryPool>& pool)
4444
: out_(out),
4545
uri_(uri),
4646
data_type_(data_type),
@@ -56,8 +56,8 @@ BlobFormatWriter::BlobFormatWriter(const std::shared_ptr<OutputStream>& out, con
5656

5757
Result<std::unique_ptr<BlobFormatWriter>> BlobFormatWriter::Create(
5858
const std::shared_ptr<OutputStream>& out, const std::shared_ptr<arrow::DataType>& data_type,
59-
const std::shared_ptr<FileSystem>& fs, const std::shared_ptr<MemoryPool>& pool,
60-
bool write_null_on_missing_file, bool write_null_on_fetch_failure) {
59+
bool write_null_on_missing_file, bool write_null_on_fetch_failure,
60+
const std::shared_ptr<FileSystem>& fs, const std::shared_ptr<MemoryPool>& pool) {
6161
if (out == nullptr) {
6262
return Status::Invalid("blob format writer create failed. out is nullptr");
6363
}
@@ -77,7 +77,7 @@ Result<std::unique_ptr<BlobFormatWriter>> BlobFormatWriter::Create(
7777
}
7878
PAIMON_ASSIGN_OR_RAISE(std::string uri, out->GetUri());
7979
return std::unique_ptr<BlobFormatWriter>(new BlobFormatWriter(
80-
out, uri, data_type, fs, pool, write_null_on_missing_file, write_null_on_fetch_failure));
80+
out, uri, data_type, write_null_on_missing_file, write_null_on_fetch_failure, fs, pool));
8181
}
8282

8383
Status BlobFormatWriter::AddBatch(ArrowArray* batch) {

src/paimon/format/blob/blob_format_writer.h

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -51,10 +51,14 @@ namespace paimon::blob {
5151
// https://cwiki.apache.org/confluence/display/PAIMON/PIP-35%3A+Introduce+Blob+to+store+multimodal+data
5252
class BlobFormatWriter : public FormatWriter {
5353
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.
5458
static Result<std::unique_ptr<BlobFormatWriter>> Create(
5559
const std::shared_ptr<OutputStream>& out, const std::shared_ptr<arrow::DataType>& data_type,
56-
const std::shared_ptr<FileSystem>& fs, const std::shared_ptr<MemoryPool>& pool,
57-
bool write_null_on_missing_file = false, bool write_null_on_fetch_failure = false);
60+
bool write_null_on_missing_file, bool write_null_on_fetch_failure,
61+
const std::shared_ptr<FileSystem>& fs, const std::shared_ptr<MemoryPool>& pool);
5862

5963
Status AddBatch(ArrowArray* batch) override;
6064

@@ -73,8 +77,9 @@ class BlobFormatWriter : public FormatWriter {
7377
private:
7478
BlobFormatWriter(const std::shared_ptr<OutputStream>& out, const std::string& uri,
7579
const std::shared_ptr<arrow::DataType>& data_type,
76-
const std::shared_ptr<FileSystem>& fs, const std::shared_ptr<MemoryPool>& pool,
77-
bool write_null_on_missing_file, bool write_null_on_fetch_failure);
80+
bool write_null_on_missing_file, bool write_null_on_fetch_failure,
81+
const std::shared_ptr<FileSystem>& fs,
82+
const std::shared_ptr<MemoryPool>& pool);
7883

7984
Status WriteBlob(std::string_view blob_data);
8085

0 commit comments

Comments
 (0)