Skip to content

Commit da5454a

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 e8bf1da commit da5454a

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
@@ -790,19 +790,29 @@ if(PAIMON_BUILD_TESTS)
790790
${TEST_STATIC_LINK_LIBS}
791791
${GTEST_LINK_TOOLCHAIN})
792792

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

808818
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)