fix(table): resolve index files by external path and bucket layout - #752
fix(table): resolve index files by external path and bucket layout#752JunRuiLee wants to merge 2 commits into
Conversation
8258587 to
a45c3e2
Compare
a45c3e2 to
5290b66
Compare
Index files were always read from `<table>/index/<file>`, ignoring both the
`_EXTERNAL_PATH` recorded in the index manifest and the
`index-file-in-data-file-dir` table option. A table that keeps index files
beside its bucket's data files fails to read them, e.g. a primary-key vector
search reports
failed to open ANN index file
'<table>/index/index-<uuid>-0' for range reads
while the file actually lives in the bucket directory.
Decode `_EXTERNAL_PATH` from the index manifest (Java `IndexFileMeta` SCHEMA
field 5) and add it to the write schema so a rewritten manifest keeps it — it is
currently dropped silently — add the `index-file-in-data-file-dir` option, and
resolve every index file through one place (`table/index_file_path.rs`) with two
modes:
* global, always `<table>/index`: the data-evolution global index, and vector
and full-text search over it;
* bucket-local, the data-file directory when the option is set: primary-key
vector ANN segments, primary-key full-text archives, deletion vectors, and
the dynamic-bucket hash index.
Each mode mirrors the factory Java uses for that consumer:
`DataEvolutionGlobalIndexScanner` resolves through `globalIndexFileFactory`,
while `IndexFileHandler` resolves hash, deletion-vector and primary-key vector
files through `pathFactories.get(partition, bucket)`, which selects
`IndexInDataFileDirPathFactory` when the option is set. An explicit external path
wins over both layouts, as in `toPath(IndexFileMeta)`.
For the two index kinds this crate writes itself, deletion vectors and the hash
index, reads and writes move together, so a file written here is found again:
* the data-evolution writer resolves an existing deletion vector through the
same path when merging, and writes a new one where the reader will look;
* the dynamic-bucket assigner resolves per partition and bucket for both
restore and commit. `BucketAssigner::prepare_commit_index` no longer takes an
index directory — three of its four implementations ignored it, and the
fourth now derives the layout itself;
* `TableCommit::abort` deletes a newly written index file where it was written,
mirroring Java `FileStoreCommitImpl.abort`, which deletes through
`indexFileFactory(partition, bucket)`. Deleting is best-effort, so the old
fixed path leaked the file silently instead of failing.
A bucket directory comes from the split that references the file when a split is
at hand, and otherwise from the partition and bucket being committed. Both go
through one `spec::bucket_path`, mirroring Java `FileStorePathFactory.bucketPath`:
the layout is only correct while every producer and consumer of a bucket
directory agrees byte for byte, and nothing else enforces that.
The option is immutable, as in Java, where it is annotated `@Immutable` and
`SchemaManager.checkAlterTableOption` rejects altering it: it selects the
directory index files are written to, so flipping it on a populated table would
hide every index file already written.
`$physical_files_size` now counts an `index-` prefixed file in a bucket directory
as an index file rather than dropping it, matching Java `FileType.classify`,
which maps any `index-*` basename to `BUCKET_INDEX` regardless of directory.
Classification follows the file's physical form, not the current option value, so
a file stays recognizable after the setting it was written under changes.
The BTree reader cache is keyed by the resolved path so two entries sharing a
file name cannot reuse each other's reader.
`data-file.path-directory` remains unsupported, as it is throughout this crate:
bucket paths are rooted directly at the table for data files as much as for
index files, so honoring it belongs with data-file path handling rather than
here.
5290b66 to
af2167b
Compare
|
|
Confirmed, and reachable: the vindex index builder aborts messages holding global index files, and Fixed in 88f1df1.
Both abort tests now also plant a same-named file in the other layout and assert it survives, so neither direction can regress into deleting a path the commit does not own. One thing I ran into while checking this against Java: |
`abort` resolved every index file in a commit message as bucket-local, so with `index-file-in-data-file-dir` set it looked for a data-evolution global index file beside the bucket's data files while the file sits under `<table>/index`. Deleting is best-effort, so a failed commit leaked it silently. Java `FileStoreCommitImpl.abort` has the same gap: it resolves every `newIndexFiles()` entry through `indexFileFactory(partition, bucket)`, while `SortedGlobalIndexWriter.flushIndex` writes through `globalIndexFileFactory()` and returns those metas in `DataIncrement.indexIncrement(...)`. Which layout a file was written under is a property of the file, not of the message carrying it. A deletion vector or the dynamic-bucket hash index carries no `_GLOBAL_INDEX` at all. A global index file carries one whose `_SOURCE_META` is either absent — what this crate's index builders write — or marked with `DataEvolutionIndexSourceMeta`'s magic, which Java added for exactly this question: "the marker distinguishes this metadata from primary-key index source metadata", and `PrimaryKeyIndexSourceMeta` starts with its own version instead. Anything else stays bucket-local, which is what Java assumes for every index file. Both abort tests now plant a same-named file in the other layout and assert it survives, so neither direction can regress into deleting a path the commit does not own. The frames the classification tests use are the ones Java serializes, and the primary-key frame is fed through `PrimaryKeyIndexSourceMeta::deserialize`, so a frame the classifier sends bucket-local is one a reader accepts.
8f92627 to
88f1df1
Compare
What
Index files are always read from
<table>/index/<file>, which ignores two things Java records andhonors:
_EXTERNAL_PATH—IndexFileMeta.SCHEMAfield 5. Not decoded here at all, so it is also droppedwhen this crate rewrites an index manifest.
index-file-in-data-file-dir— when set, Java'sindexFileFactory(partition, bucket)resolves anindex file against the bucket's data-file directory instead of the table
index/directory.Either one makes a table's index files unreadable. Observed as a primary-key vector search:
failed to open ANN index file '.../<table>/index/index-<uuid>-0' for range reads, while the file isin the bucket directory.
How
Decode
_EXTERNAL_PATHand add it to the write schema, add theindex-file-in-data-file-diroption,and route every index-file consumer through one resolver (
table/index_file_path.rs) with two modes,each mirroring the factory Java uses for that consumer:
<table>/indexglobalIndexFileFactory()pathFactories.get(partition, bucket)An explicit external path wins over both layouts, as in
toPath(IndexFileMeta).For the two index kinds this crate writes itself, reads and writes move together, so a file written
here is found again: the data-evolution writer resolves an existing deletion vector through the same
path when merging and writes a new one where the reader will look; the dynamic-bucket assigner resolves
per partition and bucket for both restore and commit (
prepare_commit_indexno longer takes an indexdirectory — three of its four implementations ignored it); and
TableCommit::abortdeletes where thefile was written, mirroring
FileStoreCommitImpl.abort. Deleting is best-effort, so the old fixed pathleaked silently — and
try_commitcallsabortitself on failure, as do the C and Python bindings.Bucket directories now come from one
spec::bucket_path(JavaFileStorePathFactory.bucketPath),replacing four copies of the same expression: the layout is only correct while every producer and
consumer agrees byte for byte, and nothing else enforces that.
Two smaller consequences of making the option live:
@Immutable, rejected bySchemaManager.checkAlterTableOption).It selects where index files are written, so flipping it on a populated table would hide every index
file the table has. It was inert here before, so altering it used to be a harmless no-op.
$physical_files_sizecounts anindex-prefixed file in a bucket directory as an index fileinstead of dropping it, matching Java
FileType.classify. Classification follows physical form, notthe current option value. Two tests that asserted the old counts are corrected.
The BTree reader cache is keyed by the resolved path — with the bare file name, two entries sharing a
name but resolving to different locations would reuse each other's reader.
Out of scope
data-file.path-directory: unsupported crate-wide (bucket paths are rooted at the table for datafiles as much as index files), so honoring it belongs with data-file path handling.
data files there either. Reading a Java-written external index file works.
higher-stakes direction (
LocalOrphanFilesCleanmaps candidate deletions byPath.getName()), andall names are UUID-based, so collisions are not reachable.
Behavior change
A table created with the option set but written only by older paimon-rust has its index files under
<table>/index, because this crate ignored the option — such a table is already unreadable by Java.After this PR Rust agrees with Java. No compatibility fallback on purpose: one would diverge from Java
and mask genuine missing-file errors.
Testing
New:
abortdeletes an index file in the bucket data-file directory, and one at an external path (bothverified to fail against the old path); ANN segment and full-text archive resolution with the option
set, through a real split whose bucket path is not derivable from the table root — the failure this PR
fixes, previously untested through a split;
directory()agrees withresolve()in every mode; decodetests for
_EXTERNAL_PATHpresent, present-but-null, and absent from the writer schema; the optioncannot be altered.
Gates:
cargo fmt --all -- --checkclean;clippy -D warnings --all-targetsclean forpaimon,paimon --features fulltext,paimon-datafusion,paimon-c(paimon-pythonneeds Python ≥ 3.10,unavailable locally);
cargo test -p paimon --lib2425 passed, 2500 with--features fulltext,--testsall pass.paimon-datafusion339 passed / 7 failed — all seven areTableNotExistfor the/tmp/paimon-warehousefixtures thatmake docker-upprovisions, unrelated to this change.