feat(core): read source-backed primary-key BTree indexes - #194
feat(core): read source-backed primary-key BTree indexes#194wangyong9999 wants to merge 12 commits into
Conversation
Add the core types for Paimon 2.0 primary-key source-backed scalar indexes: PrimaryKeyIndexSourceMeta v1 decoding (big-endian layout, Java modified UTF-8 file names, defensive count cap, trailing-byte rejection), the COMPACT level>0 source policy, pk-btree / pk-bitmap / pk-vector / pk-full-text definition parsing with field-scoped JSON option validation, and the exact per-level source group validation (PkSortedIndexGroup / PkSortedBucketIndexState) that decides whether a payload covers its data level. The metadata carrier (GlobalIndexMeta source_meta, commit message v12) landed in apache#179; this change decodes and validates what it carries. Planning and reading follow in the next part. part of apache#192
Wire the source-backed scalar indexes into ordinary batch scans of primary-key tables, mirroring the Java release-2.0.0 planner: organize same-snapshot data splits and index manifest ADD entries into validated groups, evaluate the indexed part of the scan predicate once per group with a query cache, localize group ordinals to per-file physical row positions by the source row-count prefix, and convert results to indexed splits with the 4096-range fragmentation guard, deletion files kept aligned by file index, and per-file fallback on any untrusted state. The raw read path accepts file-local row ranges (intersected with file index selection, deletion vectors subtracted) and KeyValueTableRead routes indexed splits through it; the reader still applies the complete original predicate. AND may narrow partially, OR is used only when every branch is evaluable, and redundant IS NOT NULL leaves are pruned under AND. Only the BTree payload reader is wired; bitmap / vector / full-text definitions are recognized and conservatively fall back to normal scans. Gated by global-index.enabled (default true). Integration tests arrive with the payload builder in the next part. part of apache#192
Add PkSortedIndexFile::Build, which writes exactly one BTree payload for an ordered source group from value-sorted input and returns an IndexFileMeta carrying the serialized PrimaryKeyIndexSourceMeta, plus the end-to-end integration tests that exercise the full cycle with real BTree payloads: build -> plan -> evaluate -> localize -> splits, covering equality narrowing, ranges crossing file boundaries, empty hits omitting files, fallback on unindexed columns / uncovered files / corrupted metadata / out-of-range ordinals / over-fragmented results, deletion file alignment, and same-snapshot rejection. A user guide page documents the table requirements, semantics and current scope. close apache#192
|
cc @lxy-9602 @lucasfang could you please take a look when convenient? Thanks~ |
# Conflicts: # src/paimon/core/table/source/table_scan.cpp
| // Readers are created per payload group and may coexist for many buckets. Share the | ||
| // process-wide executor instead of creating a dedicated thread pool for every group. | ||
| std::shared_ptr<Executor> executor = GetGlobalDefaultExecutor(); | ||
| return std::make_shared<LazyFilteredBTreeReader>(read_buffer_size, files, key_type, file_reader, |
There was a problem hiding this comment.
@lszskye Please evaluate whether using GlobalDefaultExecutor here could cause any issues. I’d lean toward passing the same executor to each reader instead.
Also, the executor thread count in Java is based on GLOBAL_INDEX_THREAD_NUM, rather than using the machine core count as GetGlobalDefaultExecutor() does.
A more complete approach would be to let C++ GlobalIndexer: accept an external executor and, like Java, have the scan layer create and pass it in based on global-index.thread-num, instead of having BTreeGlobalIndexer implicitly choose GetGlobalDefaultExecutor().
There was a problem hiding this comment.
Updated. GlobalIndexer::CreateReader now accepts an external executor. PrimaryKeyIndexBatchScan creates one scan-scoped executor from global-index.thread-num and shares it across BTree readers; when unset, it preserves the existing C++ CPU-count default. The legacy GlobalIndexScanImpl path keeps its outer executor separate to avoid nested synchronous work on the same fixed-size pool. No executor is created when the plan has no valid index group.
| /// four-byte standard UTF-8 form. | ||
| class JavaModifiedUtf8 { | ||
| public: | ||
| JavaModifiedUtf8() = delete; |
There was a problem hiding this comment.
This seems to be for binary compatibility of the source data file name in PrimaryKeyIndexSourceMeta, but in Java, under what scenarios would the file name contain Chinese characters or other special characters?
There was a problem hiding this comment.
I think I have a rough understanding of the issue now. It’s not limited to this spot—FileIndexFormat, DataSplit.bucketPath, and DeletionFile.path all have similar problems with Chinese characters when converting string to UTF.
I’d suggest removing this part from the current PR for now, and then submitting a separate PR later to fully address all writeUTF / readUTF related issues.
There was a problem hiding this comment.
Configured data-file.prefix can introduce Unicode. However, this is a stream-wide issue, so the PR-local codec has been removed and source metadata now uses the existing stream primitives. A separate change can address writeUTF / readUTF consistently across FileIndexFormat, DataSplit.bucketPath, DeletionFile.path, and source metadata. The remaining supplementary-code-point limitation is documented.
|
|
||
| PrimaryKeyIndexDefinition(std::string column, int32_t field_id, std::string index_type, | ||
| std::map<std::string, std::string> options, Family family) | ||
| : column_(std::move(column)), |
There was a problem hiding this comment.
Could we move the Family family parameter before options?
There was a problem hiding this comment.
Updated. Family family now precedes options; the member and accessor order and all call sites were changed consistently.
| } | ||
| } | ||
| return Status::OK(); | ||
| } |
There was a problem hiding this comment.
I’m a bit curious whether ValidateNoDuplicates and ValidateUniqueColumns could be refactored into a shared helper function, with different error reporting as needed. Also, could we move the output parameter to the end of the parameter list?
There was a problem hiding this comment.
Updated. Both validation paths now reuse AddUniqueColumns with a caller-supplied duplicate-error callback, and the output set parameter is last.
| if (ObjectUtils::Contains(btree_columns, column)) { | ||
| Result<std::map<std::string, std::string>> definition_options = | ||
| SortedIndexOptions(options, column, kBTreeOptionFamily, kBTreeAlgorithmPrefix); | ||
| PAIMON_RETURN_NOT_OK(definition_options.status()); |
There was a problem hiding this comment.
Please use ASSERT_OK_AND_ASSIGN instead of calling PAIMON_RETURN_NOT_OK first and then accessing value(). If there are similar cases elsewhere, could you fix them as well?
There was a problem hiding this comment.
Updated. This is production code, so it now uses PAIMON_ASSIGN_OR_RAISE rather than the test-only ASSERT_OK_AND_ASSIGN. Similar patterns in the new tests were changed to ASSERT_OK_AND_ASSIGN or EXPECT_OK_AND_ASSIGN.
| ASSERT_OK_AND_ASSIGN(std::unique_ptr<TableSchema> schema, | ||
| MakeSchema({{Options::PK_BTREE_INDEX_COLUMNS, "price,price"}})); | ||
| ASSERT_NOK(PrimaryKeyIndexDefinitions::Create(*schema)); | ||
| } |
There was a problem hiding this comment.
Please make the error message explicit. I’d recommend using ASSERT_NOK_WITH_MSG.
There was a problem hiding this comment.
Updated. Both duplicate-column tests now use ASSERT_NOK_WITH_MSG and check the exact diagnostics.
| out->push_back(static_cast<char>((bits >> shift) & 0xFF)); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Please try to reuse DataOutputStream, DataInputStream, MemorySegmentOutputStream for endianness conversion and data input/output.
There was a problem hiding this comment.
Updated. Deserialization now uses ByteArrayInputStream with DataInputStream, and serialization uses MemorySegmentOutputStream for pooled big-endian output. DataOutputStream cannot wrap MemorySegmentOutputStream without an adapter or extra copy. The string length and bytes remain explicit because MemorySegmentOutputStream::WriteString narrows through int16_t, while this wire field supports the full uint16_t range. Boundary and null-input tests were added.
| std::optional<std::string> external_path; | ||
| if (is_external_path) { | ||
| external_path = io_meta.file_path; | ||
| } |
There was a problem hiding this comment.
Please change this to something like:
if (is_external_path) {
PAIMON_ASSIGN_OR_RAISE(Path path, PathUtil::ToPath(io_meta.file_path));
external_path = path.ToString();
}Could we normalize the path here? Regular global index writing does this in global_index_write_task.cpp. In Java, org.apache.paimon.fs.Path parses and normalizes the URI during construction, so the final stored value is the normalized form from Path.toString().
| const std::shared_ptr<arrow::Array>& sorted_values, std::vector<int64_t> sorted_ordinals, | ||
| const std::shared_ptr<GlobalIndexFileWriter>& file_writer, bool is_external_path, | ||
| const std::shared_ptr<MemoryPool>& pool) { | ||
| PAIMON_ASSIGN_OR_RAISE(PrimaryKeyIndexSourceMeta source_meta, |
There was a problem hiding this comment.
The current implementation puts all planned sorted data for an entire index file into a single sorted_values, which could lead to excessive memory usage. While Java uses an external sort buffer here.
Given the scope of the current PR, I’d suggest adding a TODO to clearly document this as a known issue and plan to fix it in a follow-up PR.
| Result<std::unique_ptr<BatchReader>> CreateReader( | ||
| const BinaryRow& partition, int32_t bucket, | ||
| const std::vector<std::shared_ptr<DataFileMeta>>& files, DeletionVector::Factory dv_factory, | ||
| const std::optional<std::vector<Range>>& local_row_ranges = std::nullopt); |
There was a problem hiding this comment.
Please avoid using default arguments in production code.
There was a problem hiding this comment.
Also, I’m not sure it’s necessary to add a new CreateReader function. Could we just extend the existing one with a local_row_ranges parameter instead?
| if (inner_split_impl->DataFiles().size() != 1) { | ||
| return Status::Invalid( | ||
| "indexed splits with file-local row ranges must contain exactly one file"); | ||
| } |
There was a problem hiding this comment.
If the indexed split contains scores, is read supported right now? I don’t see any handling logic for that. If it’s not supported, please fail fast and add a TODO to mark it clearly.
| builder.WithSnapshot(source->SnapshotId()) | ||
| .WithTotalBuckets(source->TotalBuckets()) | ||
| .IsStreaming(false) | ||
| .RawConvertible(source->RawConvertible()); |
There was a problem hiding this comment.
It seems in Java, rawConvertible is always false here. The current implementation would make the scan results inconsistent with Java.
| public: | ||
| FilePlan(std::shared_ptr<DataSplitImpl> source_split, int32_t file_index, | ||
| std::map<int32_t, std::shared_ptr<PkSortedIndexGroup>> groups) | ||
| : source_split_(std::move(source_split)), |
There was a problem hiding this comment.
PkSortedIndexGroup::Create returns optional<PkSortedIndexGroup>, but the call sites seem to use std::shared_ptr<PkSortedIndexGroup>. Should we adjust the return type of Create for consistency?
| CONTAINS, | ||
| LIKE, | ||
| }; | ||
|
|
There was a problem hiding this comment.
Could you help clarify the distinction between QueryOperation and Function? They seem somewhat overlapping to me, so I’m wondering why both are needed separately.
| scalar_definitions.push_back(definition); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Could ScalarDefinitions be handling something similar here?
| ordinals[1] = 0; | ||
| ASSERT_NOK_WITH_MSG(BuildPayload(std::move(ordinals)).status(), | ||
| "Row id 0 appears more than once"); | ||
| } |
There was a problem hiding this comment.
ASSERT_NOK_WITH_MSG(Func(), error_message); is OK here.
| Result<PrimaryKeySortedIndexScan::Plan> plan = PrimaryKeySortedIndexScan::CreatePlan( | ||
| kSnapshotId, {split}, definitions_, MakeEntries(payload)); | ||
| ASSERT_NOK(plan.status()); | ||
| } |
There was a problem hiding this comment.
ASSERT_NOK(PrimaryKeySortedIndexScan::CreatePlan(
kSnapshotId, {split}, definitions_, MakeEntries(payload)));
| ASSERT_NOK(plan.status()); | ||
| } | ||
|
|
||
| } // namespace paimon::test |
There was a problem hiding this comment.
The newly added tests mainly cover internal logic such as CreatePlan, Evaluate, and ToSplits. Please also add E2E tests similar to test/inte/global_index_test.cpp, covering the complete path:
Java writes the table and PK BTree index
→ C++ TableScan::CreatePlan
→ IndexedSplit / DataSplit
→ TableRead::CreateReader
→ validate the returned rows
These tests should not only invoke PkSortedIndexFile::Build, PrimaryKeySortedIndexScan::Evaluate, or mock readers directly. Otherwise, they do not cover Java/C++ format compatibility, manifest/source metadata parsing, index reader creation, split conversion, and actual data-file reading.
Please consider covering the following scenarios:
-
Index written by Java and read by C++
Let Java write a primary-key table with a
pk-btreeindex, then query it from C++ using a predicate on the indexed field.In addition to validating the returned rows, verify that the scan plan contains an
IndexedSplitwith the expected single data file and file-local row ranges. -
A mixture of indexed and non-indexed files
First compact the table to generate indexed files, then append new L0 files without another compaction. The query range should match rows from both:
- a compacted file covered by a PK BTree index;
- a newly appended L0 file without an index.
Verify that the plan contains both
IndexedSplitand regularDataSplitinstances, and that the final read returns rows from both groups without false negatives. -
One index payload covering multiple source data files
Use Java to perform a compaction that produces multiple data files while creating only one PK BTree index group. The query result should span at least two source files.
Verify that C++ correctly converts group-global row ordinals into file-local row ranges and creates the corresponding single-file
IndexedSplitinstances. ExecuteTableReadand verify that matching rows from all source files are returned. -
Deletion Vector together with a residual predicate
Let Java compact the table and build the index, then perform updates or deletes to generate a deletion vector. Use a predicate containing both an indexed and a non-indexed field, for example:
score = 10 AND tag = 'keep'Verify that the PK BTree row ranges, deletion vector, and complete residual predicate are all applied. Deleted rows and rows that fail the residual predicate must not be returned.
-
Safe fallback for AND/OR predicates
Test both:
score = 10 AND tag = 'keep' score = 10 OR tag = 'keep'For the AND predicate, the
scoreindex may narrow the row ranges whiletagremains a residual predicate.For the OR predicate, if one branch cannot be evaluated by the index, the scan must not prune data using only the indexed branch. It should safely fall back to a regular scan. Both cases should execute the complete scan-and-read path and validate the final rows.
-
Global index disabled
Set the scan option:
{{Options::GLOBAL_INDEX_ENABLED, "false"}}or equivalently:
global-index.enabled=falseVerify that no
IndexedSplitis produced, the regular scan/read path is used, and the returned rows are identical to those returned with the index enabled. -
Historical snapshots and index versions
Read the following snapshots from the same Java-generated fixture:
- snapshot 2: use the index generated by the first compaction;
- snapshot 3: use the old index for the compacted file and fall back for the new L0 file;
- snapshot 5: use the index rebuilt by the later compaction.
Validate both the plan and the final rows for each snapshot. This should ensure that C++ only applies index payloads belonging to the requested snapshot and matching the exact source files, rather than incorrectly applying the snapshot 5 index to files from snapshot 2 or 3.
-
Partitions and multiple buckets
Prepare a Java fixture with multiple partitions and multiple buckets, with PK BTree index groups in different buckets. The query should match rows across partitions and buckets.
Verify that each
IndexedSplithas the correct partition, bucket, source data file, and index-file path, and validate the final read result. -
Routing
IndexedSplitthroughFallbackTableReadConfigure
scan.fallback-branchand create a scenario where the main branch produces anIndexedSplit.Verify that:
- an
IndexedSplitfrom the main branch is sent to the main table reader; - a split from the fallback branch is sent to the fallback table reader;
- an
IndexedSplitis not rejected or routed to the wrong reader because of its split type; - the final read result is correct.
- an
-
Both Parquet and ORC
Please generate equivalent Java fixtures for Parquet and ORC and parameterize the tests with
TEST_P, for example:using ParamType = std::string; INSTANTIATE_TEST_SUITE_P( FileFormat, PrimaryKeySortedIndexE2ETest, ::testing::Values("parquet", "orc"));
Every case should cover the actual
TableScan + TableReadpath rather than only validating the plan. Depending on the scenario, the tests should also verify the split type, source data file, file-local row ranges, and deletion file.
These tests could be added to a dedicated primary_key_sorted_index_inte_test.cpp or to the existing global_index_test.cpp. The Java-generated fixtures should be stored under test/test_data/{parquet,orc}/..., with a README documenting the schema, writes, compactions, and expected state of each snapshot.
Purpose
Linked issue: Closes #192
This PR adds end-to-end read-side integration for source-backed primary-key BTree indexes introduced in Apache Paimon Java release-2.0.0. It consolidates the originally planned three stacked parts into one self-contained change.
The main changes are:
PrimaryKeyIndexSourceMetav1 with the Java-compatible big-endian layout and Java modified UTF-8 file names. Unknown versions, malformed counts, truncation, and trailing bytes are rejected.pk-btree,pk-bitmap,pk-vector, andpk-full-textcolumn-definition options and reject duplicate or cross-family assignments. For BTree and Bitmap, resolve Java-compatible field-scoped JSON options, including scalar-value coercion; vector index types are also carried in their definitions.PkSortedIndexGroupandPkSortedBucketIndexState. A payload is trusted only when exactly one candidate matches the active orderedCOMPACTfiles, row counts, field ID, index type, and complete row range.PkSortedIndexFile::Buildfor constructing one BTree payload from value-sorted input, together with end-to-end coverage for build -> plan -> evaluate -> localize -> split.The planner is deliberately conservative.
ANDpredicates can narrow with safely evaluable children, whileORpredicates use the index only when every branch is evaluable. Missing or corrupt metadata and incomplete coverage leave the affected files on normal scans. An out-of-range group ordinal invalidates localization for every covered file in that group; invalid file-local positions or a result requiring more than 4096 ranges cause the affected file to fall back. Indexed splits keep deletion files aligned, and the reader still evaluates the complete original predicate, so the optimization does not change visibility semantics.The optimization applies only to snapshot-scoped, non-read-optimized primary-key batch scans outside the Data Evolution path. Splits that cannot be converted to raw file-local reads retain their original form.
Only the BTree payload reader is wired into scan planning in this PR. Bitmap, vector, and full-text option keys are recognized, but this change adds no payload reader for them, so their scans remain on the ordinary path.
PkSortedIndexFile::Buildprovides the payload-building primitive exercised by tests and available to tooling; automatic payload build and maintenance during compaction are outside this change.Tests
ci/scripts/test_cmake_modules.sh; all AArch64-marchand target-architecture checks passed.paimon-memory-testandpaimon-core-testwith GCC 8.3.0, Debug mode, andPAIMON_BUILD_TESTS=ON.JavaModifiedUtf8Testcases.PrimaryKeyIndexSourceMetaTestcases.PrimaryKeyIndexDefinitionsTestcases.PkSortedBucketIndexStateTestcases.FallbackTableReadTestrouting case.PrimaryKeySortedIndexScanTestend-to-end cases.paimon-memory-testand all 1,673paimon-core-testcases.API and Format
pk-btree.index.columns,pk-bitmap.index.columns,pk-vector.index.columns, andpk-full-text.index.columns, matching JavaCoreOptions.GlobalIndexMeta._SOURCE_METAcarrier added in feat: update commit message to version 12 #179 and is byte-compatible with the Java release-2.0.0PrimaryKeyIndexSourceMetav1 contract.Documentation
Adds
docs/source/user_guide/primary_key_global_index.rstand links it from the user guide.Generative AI tooling
Generated-by: OpenAI Codex (GPT-5) and Claude Code (Fable 5)