Skip to content

feat: support to read chain data split#387

Open
Weixin-Xu wants to merge 5 commits into
alibaba:mainfrom
Weixin-Xu:support_chain_table_oss
Open

feat: support to read chain data split#387
Weixin-Xu wants to merge 5 commits into
alibaba:mainfrom
Weixin-Xu:support_chain_table_oss

Conversation

@Weixin-Xu

@Weixin-Xu Weixin-Xu commented Jun 29, 2026

Copy link
Copy Markdown

Purpose

Linked issue: close #385

Support deserializing, serializing, and reading community-format ChainSplit.

This change adds ChainSplit support for the current Java SplitSerializer wire format, including:

  • recognizing the SplitSerializer magic, serializer version, and CHAIN_SPLIT type
  • deserializing ChainSplit fields: logical partition, data files, file bucket path mapping, and file branch mapping
  • serializing ChainSplit back through SplitSerializer without dropping chain metadata
  • resolving ChainSplit data file paths through per-file bucket path mapping
  • failing fast when a non-external ChainSplit file is missing bucket path mapping
  • preserving existing DataSplit, FallbackDataSplit, and IndexedSplit behavior

Tests

  • Added ChainSplitTest
    • deserialize community-format ChainSplit bytes
    • deserialize ChainSplit bytes with multiple data files
    • verify per-file bucket path resolution
    • verify external file path is preserved
    • verify missing bucket path mapping returns an error
    • verify ChainSplit serialization preserves bucket and branch mappings
    • verify malformed ChainSplit bytes return an error

API and Format

This change does not modify table storage format. It updates split protocol support to handle the community Java ChainSplit format.

The read path consumes fileBucketPathMapping for per-file bucket path resolution. It also deserializes and preserves fileBranchMapping for format compatibility; branch-aware read/schema selection is intentionally deferred to a follow-up change.

Documentation

No user-facing documentation update.

Generative AI tooling

Generated-by: OpenAI Codex

@CLAassistant

CLAassistant commented Jun 29, 2026

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@Weixin-Xu Weixin-Xu changed the title Support chain data split Support to read chain data split Jun 29, 2026

@zjw1111 zjw1111 left a comment

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.

Thanks for adding ChainDataSplit support! The overall design looks good. A few minor suggestions below.

Result<std::unique_ptr<BatchReader>> ApplyPredicateFilterIfNeeded(
std::unique_ptr<BatchReader>&& reader, const std::shared_ptr<Predicate>& predicate) const;

Result<std::shared_ptr<DataFilePathFactory>> CreateDataFilePathFactory(

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.

Minor: CreateDataFilePathFactory is currently in the public section, but it seems to only be called by subclasses internally. Would it be possible to move it into the protected section just below?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Fixed in the latest revision. CreateDataFilePathFactory is now protected since it is only used by AbstractSplitRead subclasses.


const std::unordered_map<std::string, std::string>& FileBucketPathMapping() const {
return file_bucket_path_mapping_;
}

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.

I noticed that file_branch_mapping_ is deserialized and stored, but not consumed by any read path in this PR (e.g. ChainDataFilePathFactory only uses file_bucket_path_mapping_). I assume this is reserved for future use (e.g. selecting the correct schema per branch, as the Java side does). Could you add a brief note in the PR description mentioning this is intentionally deferred?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Added a note in the API and Format section to clarify that file_branch_mapping is preserved for ChainDataSplit metadata compatibility, while branch-aware read/schema selection is deferred. The current read path only consumes file_bucket_path_mapping.

@zjw1111 zjw1111 changed the title Support to read chain data split feat: support to read chain data split Jun 30, 2026
if (!data_file_path_factory) {
PAIMON_ASSIGN_OR_RAISE(data_file_path_factory,
path_factory_->CreateDataFilePathFactory(partition, bucket));
}

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.

This design feels a bit too implicit. Why can’t each caller just pass in the correct data_file_path_factory directly?

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.

Also, let’s avoid using default arguments in production code.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Fixed. The callers now construct the appropriate DataFilePathFactory explicitly before delegating:

  • split-based reads use CreateDataFilePathFactory(data_split), so ChainDataSplit can get the chain-aware factory.
  • partition/bucket reads create the normal factory with path_factory_->CreateDataFilePathFactory(partition, bucket).

The private helper now requires a DataFilePathFactory argument directly, so there is no nullptr fallback and no default argument.

Comment thread src/paimon/core/table/source/split.cpp Outdated
return Status::Invalid(fmt::format("invalid ChainDataSplit byte stream: {}",
chain_split.status().ToString()));
}
return std::static_pointer_cast<Split>(chain_split.value());

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.

Could you please use PAIMON_ASSIGN_OR_RAISE rather than if (!chain_split.ok()).

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Fixed. Replaced the manual chain_split.ok() checks with PAIMON_ASSIGN_OR_RAISE in both ChainDataSplit tail parsing paths.

Comment thread src/paimon/core/table/source/split.cpp Outdated

Result<std::shared_ptr<ChainDataSplitImpl>> ReadChainDataSplitTail(
const std::shared_ptr<DataSplitImpl>& base_split, DataInputStream* in,
const std::shared_ptr<MemoryPool>& pool) {

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.

Could you move the output parameter (in) in to the end of the parameter list?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Fixed. Moved DataInputStream* in to the end of ReadChainDataSplitTail's parameter list and updated both call sites.

struct DataFileMeta;
namespace {
Result<std::vector<std::shared_ptr<DataFileMeta>>> ReadVersion7DataFileMetaList(
DataInputStream* in, const std::shared_ptr<MemoryPool>& pool) {

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.

I found that the chain table split is currently not compatible with the Java implementation. Java uses ChainSplitHeader + logicalPartition + files + bucketMap + branchMap, while C++ uses DataSplit + ChainTail. The split format must be kept fully consistent with Java.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Yes, I was working from an outdated commit. I’ll realign the implementation with the current community Java format, then submit an updated patch after further changes and verification.

@Weixin-Xu Weixin-Xu force-pushed the support_chain_table_oss branch from 338b67a to 8332390 Compare July 9, 2026 06:57
@Weixin-Xu Weixin-Xu force-pushed the support_chain_table_oss branch from 4b4e81b to 98d0811 Compare July 9, 2026 07:31
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.

[Bug] Chain Table read failure

4 participants