Skip to content

feat(blob): support reading map blob values - #278

Open
XiaoHongbo-Hope wants to merge 12 commits into
apache:mainfrom
XiaoHongbo-Hope:codex/read-map-blob
Open

feat(blob): support reading map blob values#278
XiaoHongbo-Hope wants to merge 12 commits into
apache:mainfrom
XiaoHongbo-Hope:codex/read-map-blob

Conversation

@XiaoHongbo-Hope

@XiaoHongbo-Hope XiaoHongbo-Hope commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Purpose

Linked issue: #283

Support reading Java-compatible MAP<K, BLOB> payloads from blob files through the real Table API and data-evolution paths.

  • Decode BOOL, integer, DATE, STRING, BINARY, and DECIMAL keys.
  • Load top-level MAP<K, BLOB> schemas from table JSON while continuing to reject BLOBs in other nested positions.
  • Treat existing Java MAP<K, BLOB> tables as read-only in C++: reject C++ table creation, FileStoreWrite::Create, and append compaction until writer support is implemented.
  • Resolve MAP<K, BLOB> placeholders across multiple sequence layers in both inline and descriptor modes.
  • Preserve null and empty maps/values, and reject malformed metadata, duplicate keys, non-canonical large DECIMAL encodings, invalid UTF-8 STRING keys, and null map keys.
  • Reject paimon.map.selected-keys for Map Blob reads because filtering can remove the internal data-evolution fallback placeholder.

TIME keys and ARRAY<BLOB> are intentionally outside this PR and should be handled separately with their end-to-end type and format support.

Tests

  • ninja -C build-release paimon-blob-format-test
  • build-release/release/paimon-blob-format-test --gtest_filter='BlobFileBatchReaderTest.*:BlobAsDescriptor/BlobFileBatchReaderTest.*' (17 tests passed)
  • Focused NestedProjectionUtilsTest.GetMapSelectedKeysRejectsMapBlob test (passed)
  • Changed production and test translation units compiled with -Werror -Wall
  • cpplint, codespell, and git diff --check

API and Format

No public API under include/ and no storage format changes. This adds read compatibility for the existing Paimon Java MAP<X, BLOB> payload version 1. C++ writer support is unchanged; table creation, regular writes, and append compaction are explicitly rejected.

Arrow Map requires non-null keys, so Java Map Blob payloads containing null keys are rejected during decoding.

Documentation

No documentation changes.

Generative AI tooling

Generated-by: OpenAI Codex (GPT-5)

@XiaoHongbo-Hope
XiaoHongbo-Hope marked this pull request as ready for review September 3, 2026 05:46
@duanyyyyyyy

Copy link
Copy Markdown

@lxy-9602 pls take time to review

Comment thread src/paimon/common/types/data_type.cpp Outdated
Comment thread src/paimon/format/blob/blob_file_batch_reader.cpp
Comment thread src/paimon/format/blob/blob_file_batch_reader.cpp
Comment thread src/paimon/format/blob/blob_file_batch_reader.cpp Outdated

Result<std::shared_ptr<arrow::Array>> BlobFileBatchReader::BuildMapBlobArray(
int32_t rows_to_read) const {
const auto& struct_type = static_cast<const arrow::StructType&>(*target_type_);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function seems too long. Please break it down into smaller, more fine-grained pieces.

@XiaoHongbo-Hope XiaoHongbo-Hope Sep 4, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refactored in 00da44a. Payload parsing/validation, key appends, and value appends are now split into ReadMapBlobPayload, AppendMapBlobKeys, and AppendMapBlobValues; BuildMapBlobArray only coordinates rows and builders.

Comment thread src/paimon/format/blob/blob_file_batch_reader_test.cpp
ASSERT_OK_AND_ASSIGN(int64_t written,
output_stream->Write(file_bytes.data(), file_bytes.size()));
ASSERT_EQ(file_bytes.size(), written);
ASSERT_OK(output_stream->Close());

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider use file_system->WriteFile

@XiaoHongbo-Hope XiaoHongbo-Hope Sep 4, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated all added golden/corruption fixture writes to use FileSystem::WriteFile in 00da44a.

Comment thread src/paimon/format/blob/blob_file_batch_reader_test.cpp Outdated
Comment thread src/paimon/format/blob/blob_file_batch_reader.cpp
for (int32_t entry = 0; entry < entry_count; ++entry) {
const auto key_length = static_cast<int32_t>(key_lengths[entry]);
std::vector<uint8_t> key_bytes(key_length);
PAIMON_RETURN_NOT_OK(ReadBlobContentAt(key_offset, key_length, key_bytes.data()));

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’m not sure how large key_bytes can get. If it can be large, it would be better to allocate it from the pool, for example via Bytes.

@XiaoHongbo-Hope XiaoHongbo-Hope Sep 4, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated in 00da44a. Key buffers now use pool-backed Bytes; inline value buffers use the same allocation path.

@lxy-9602

lxy-9602 commented Sep 4, 2026

Copy link
Copy Markdown
Member

Thank you very much for adding support for map<..., blob>. Would it be possible to open an issue to describe this feature? I’d also like to know whether there is a plan to support writes in the future.

@zjw1111

zjw1111 commented Sep 4, 2026

Copy link
Copy Markdown
Collaborator

Thanks for your work on this! Is there any plan to support reading and writing ARRAY<BLOB> in a follow-up?

{"TIME(0)", arrow::time32(arrow::TimeUnit::MILLI)},
{"TIME(3)", arrow::time32(arrow::TimeUnit::MILLI)},
{"TIME(9)", arrow::time32(arrow::TimeUnit::MILLI)},
{"TIME(3) WITHOUT TIME ZONE", arrow::time32(arrow::TimeUnit::MILLI)},

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please also add ut for "TIME(6) WITHOUT TIME ZONE"...

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please also add ut for "TIME(6) WITHOUT TIME ZONE"...

Removed TIME support in this PR.

@XiaoHongbo-Hope

XiaoHongbo-Hope commented Sep 4, 2026

Copy link
Copy Markdown
Contributor Author

@lxy-9602 I opened #283 to describe the feature and current compatibility boundaries. This PR remains read-only; there is no concrete Map Blob writer timeline yet, current case ready only is enough.

@zjw1111 There is no concrete ARRAY<BLOB> plan yet.

lxy-9602
lxy-9602 previously approved these changes Sep 4, 2026

@lxy-9602 lxy-9602 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Let’s wait for @lszskye to confirm as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants