feat: support to read chain data split#387
Conversation
zjw1111
left a comment
There was a problem hiding this comment.
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( |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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_; | ||
| } |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
| if (!data_file_path_factory) { | ||
| PAIMON_ASSIGN_OR_RAISE(data_file_path_factory, | ||
| path_factory_->CreateDataFilePathFactory(partition, bucket)); | ||
| } |
There was a problem hiding this comment.
This design feels a bit too implicit. Why can’t each caller just pass in the correct data_file_path_factory directly?
There was a problem hiding this comment.
Also, let’s avoid using default arguments in production code.
There was a problem hiding this comment.
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.
| return Status::Invalid(fmt::format("invalid ChainDataSplit byte stream: {}", | ||
| chain_split.status().ToString())); | ||
| } | ||
| return std::static_pointer_cast<Split>(chain_split.value()); |
There was a problem hiding this comment.
Could you please use PAIMON_ASSIGN_OR_RAISE rather than if (!chain_split.ok()).
There was a problem hiding this comment.
Fixed. Replaced the manual chain_split.ok() checks with PAIMON_ASSIGN_OR_RAISE in both ChainDataSplit tail parsing paths.
|
|
||
| Result<std::shared_ptr<ChainDataSplitImpl>> ReadChainDataSplitTail( | ||
| const std::shared_ptr<DataSplitImpl>& base_split, DataInputStream* in, | ||
| const std::shared_ptr<MemoryPool>& pool) { |
There was a problem hiding this comment.
Could you move the output parameter (in) in to the end of the parameter list?
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
338b67a to
8332390
Compare
4b4e81b to
98d0811
Compare
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:
Tests
ChainSplitTestAPI 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
fileBucketPathMappingfor per-file bucket path resolution. It also deserializes and preservesfileBranchMappingfor 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