feat(file-index): support writing file indexes - #210
Conversation
580352e to
18fe9aa
Compare
| const std::vector<MemorySegment>& segments = output_.Segments(); | ||
| result_ = std::shared_ptr<Bytes>(new Bytes(static_cast<size_t>(position_), pool_.get()), | ||
| [pool = pool_](Bytes* bytes) { delete bytes; }); | ||
| MemorySegmentUtils::CopyToBytes(segments, /*offset=*/0, result_.get(), |
There was a problem hiding this comment.
This implementation feels a bit unusual. Would it make sense to add a helper on Bytes instead, for example:
static std::shared_ptr<Bytes> AllocateShared(
size_t size, std::shared_ptr<MemoryPool> pool);There was a problem hiding this comment.
Thanks. I removed the custom deleter instead of adding AllocateShared. ByteArrayOutputStream::Finish now receives a MemoryPool* and constructs the result with std::make_shared. The API documents that the caller must keep the pool alive until the returned Bytes is destroyed, and the upper layer guarantees that lifetime. With this ownership contract, a helper that captures shared_ptr is no longer needed.
| int64_t in_manifest_threshold_; | ||
| std::shared_ptr<FileSystem> file_system_; | ||
| std::shared_ptr<DataFilePathFactory> path_factory_; | ||
| std::shared_ptr<MemoryPool> pool_; |
There was a problem hiding this comment.
Is pool_ placed last because none of the other member variables in this class use it?
There was a problem hiding this comment.
Yes, this is about destruction order. Members are destroyed in reverse declaration order. The earlier members do not rely on DataFileIndexWriter::pool_ during destruction: each current FileIndexWriter keeps its own shared_ptr. pool_ here is used directly only while serializing the container, so declaring it after writers_ is safe. If a future member borrows this pool instead of owning it, pool_ should be declared before that member.
| int64_t record_count = batch->length; | ||
| PAIMON_RETURN_NOT_OK(SingleFileWriter::Write(batch)); | ||
| PAIMON_RETURN_NOT_OK(WriteRecord(batch, batch)); | ||
| seq_num_counter_->Add(record_count); |
There was a problem hiding this comment.
The two parameters of WriteRecord are hard to understand in the current form.
| return Status::Invalid(fmt::format("Invalid file index option {}", key)); | ||
| } | ||
| for (std::string column_name : StringUtils::Split(value, ",", /*ignore_empty=*/false)) { | ||
| StringUtils::Trim(&column_name); |
There was a problem hiding this comment.
We intentionally keep ignore_empty=false so leading or middle empty column entries, such as f1,,f2, are rejected instead of silently ignored. Java String.split drops trailing empty tokens, so malformed values such as f1,f2,, currently behave differently. I reverted the ad-hoc special case and added a TODO to align this together with ConfigParser::ParseList, so list option parsing can be changed consistently.
8ae9807 to
e3fe5c7
Compare
3 participants
Purpose
Linked issue: #173
Add the File Index write path and initially support Bitmap and Range Bitmap indexes.
FileIndexFormat::WriterAPI and Java-compatible V1 index serialization.DataFileMetaand publish larger indexes as extra.indexfiles.DataFileWriterBase.ByteArrayOutputStreamon top ofMemorySegmentOutputStream.CoreOptions.Bloom Filter and BSI writers are intentionally deferred to a follow-up change.
This PR covers writer-level and rolling-writer abort cleanup. Cleanup of
DataFileMeta.extra_filesfrom snapshot expiration, commit abort, and append/merge-tree intermediate compaction paths is tracked as follow-up work.Tests
cmake --build build --target unittest -j "$(nproc)"built all unit and integration test targets. The initial run passed 26 of 27 test executables and exposed a null-buffer validation issue in the newByteArrayOutputStreamtest; the issue was fixed../build/debug/paimon-common-testpassed all 1439 tests after the fix../build/debug/paimon-core-test --gtest_filter='DataFileIndexWriterTest.*:CoreOptionsTest.*:AppendOnlyWriterTest.*'passed all 56 selected tests after relinking the final library.paimon-core-testand passed 10 focused tests covering append, key-value,DataFileIndexWriter, andFileIndexOptions.WriteAndReadInteTest.TestAppendWithExternalBitmapAndRangeBitmapIndexesto the parameterized end-to-end suite.pre-commit run --from-ref upstream/main --to-ref HEADpassed during implementation; the latest changed production files also passedclang-format --dry-run --Werrorandgit diff --check.API and Format
Yes. This adds the public
FileIndexFormat::WriterAPI and writes the Java-compatible V1 File Index container format. Existing read formats and existing public APIs are not changed incompatibly.Documentation
No user-facing documentation changes in this PR.
Generative AI tooling
Generated-by: OpenAI Codex (GPT-5)