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 3 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
| } | ||
| crate::spec::SchemaChange::SetOption { key, .. } | ||
| | crate::spec::SchemaChange::RemoveOption { key } | ||
| if key == INDEX_FILE_IN_DATA_FILE_DIR_OPTION => |
There was a problem hiding this comment.
[P1] Keep the index layout immutable for dynamic table copies too
This guard only covers catalog ALTER TABLE, but the public Table::copy_with_options path still merges index-file-in-data-file-dir through TableSchema::copy_with_options. Every new resolver then reads that overridden value. For example, a persisted true table copied with false looks for its bucket-local deletion/vector/full-text indexes under <table>/index; the inverse misses indexes stored there. More seriously, a write/commit built from such a copy can place new hash/DV index files according to the temporary value while committing name-only manifest entries to a table whose persisted schema retains the opposite value, so a normally reloaded table cannot resolve those files and abort cleanup can use the wrong layout. Please pin this option to the stored schema value (including the default false) or reject dynamic overrides, as is already done for other safety-sensitive options, and cover both read and write copies in a regression test.
There was a problem hiding this comment.
Fixed in 3df9e15. TableSchema::copy_with_options now pins the option to the stored value, and drops the override entirely when nothing is stored so the default stands — the shape it already uses for type. Java rejects such an override outright (AbstractFileStoreTable.checkImmutability); this copy is infallible, so pinning is the closest equivalent.
Tests: a copied-with-false table still writes its hash index into the bucket directory, a read through Table::copy_with_options still reports the stored layout, and the pin in both directions. copy_with_replaced_options and Table::new stay unpinned — they take options wholesale rather than as an override, as type has always been.
One correction: a writer and its committer come from the same WriteBuilder, so abort resolves the layout the write used. The damage that needs no such crossing is the successful commit itself.
| { | ||
| return Err(Error::Unsupported { | ||
| message: format!( | ||
| "changing '{INDEX_FILE_IN_DATA_FILE_DIR_OPTION}' is not supported: \ |
There was a problem hiding this comment.
[P2] Preserve Java’s snapshot-aware/no-op immutable-option behavior
This arm rejects every set/remove of index-file-in-data-file-dir, even before the table has its first snapshot and even when a SetOption repeats the stored value. Upstream Java SchemaManager first computes unchanged and only calls checkAlterTableOption when hasSnapshots && !unchanged; this lets callers choose the layout through ALTER before the first write and makes schema reconciliation/idempotent SETs succeed. The new Rust test instead cements unconditional failure on an empty table. Please allow an exact no-op, allow an actual change while there are no snapshots, and reject only an actual change once snapshots exist; add coverage for all three cases.
There was a problem hiding this comment.
Fixed in 3df9e15. The guard now takes has_snapshots and rejects only an actual change once snapshots exist; RemoveOption only with snapshots, set or not. type stays unconditional, as in Java — a format table holds data without writing a snapshot. alter_table resolves the flag only when a change touches the option.
One case that looks like a no-op and is not: false where the option was never stored. Java compares the stored value and normalizes only type, primary-key and partition, so it counts as a change and is rejected once snapshots exist. Tested, along with the four you listed.
Gating on snapshots leaves a window, now in the guard's docs: a writer that started before the alter still holds the old layout, and a commit records its schema id without checking whether the latest schema moved. Java's cached lazy check has the same one; closing it belongs on the commit path.
…ts alter on snapshots Two holes in making `index-file-in-data-file-dir` live. `Table::copy_with_options` still merged it as a dynamic override, and every resolver reads the copy's schema — the writers included. A table persisted with one value and copied with the other wrote hash and deletion-vector index files according to the copy, while committing manifest entries that record only a file name, so a normally loaded table could not resolve them. `TableSchema::copy_with_options` now pins the option to the stored value, as it already does for `type`, and drops the override entirely when nothing is stored so the default stands. Java rejects such an override outright (`AbstractFileStoreTable.checkImmutability`), but this copy cannot fail. The alter guard rejected every set and remove, including before the first write and including a `SetOption` that repeats the stored value. Java reaches `checkAlterTableOption` only under `hasSnapshots && !unchanged`, so a caller can choose the layout through ALTER before anything is written and schema reconciliation stays idempotent. The guard now takes that flag, and `alter_table` resolves it only when a change touches the option, mirroring Java's `LazyField` rather than paying a snapshot lookup on every alter. `type` stays unconditional, as it is in Java: a format table holds data without ever writing a snapshot. Setting the option to `false` where it was never stored is a change, not a no-op — Java compares the stored string and normalizes only `type`, `primary-key` and `partition` — so it is rejected once snapshots exist even though it names the layout already in use. Gating on snapshot existence leaves one window open, in Java as much as here, and the guard's documentation now says so: a writer that started before the alter still holds the old layout, and a commit records the schema id it was built with without checking whether the latest schema moved. Closing it needs schema publication and snapshot commit ordered against each other, not a stricter alter. `test_table_with_options` built its schema through `copy_with_options`, which the pin now filters, so it persists its options instead — the way a catalog-loaded table carries them.
2 participants
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.