JunRuiLee commented on code in PR #752:
URL: https://github.com/apache/paimon-rust/pull/752#discussion_r3900984029
##########
crates/paimon/src/catalog/filesystem.rs:
##########
@@ -539,7 +542,26 @@ impl Catalog for FileSystemCatalog {
full_name: identifier.full_name(),
})?;
- reject_table_type_changes(current.options(), &changes)?;
+ // Only the index-layout option is snapshot-gated, so the listing a
+ // snapshot lookup costs is paid only when a change touches it. Java
defers
+ // the same lookup through a `LazyField`.
+ let touches_snapshot_gated_option = changes.iter().any(|change| {
+ matches!(
+ change,
+ crate::spec::SchemaChange::SetOption { key, .. }
+ | crate::spec::SchemaChange::RemoveOption { key }
+ if key == INDEX_FILE_IN_DATA_FILE_DIR_OPTION
+ )
+ });
+ let has_snapshots = if touches_snapshot_gated_option {
+ SnapshotManager::new(self.file_io.clone(), table_path.clone())
+ .get_latest_snapshot_id()
+ .await?
+ .is_some()
+ } else {
+ false
+ };
+ reject_immutable_option_changes(current.options(), &changes,
has_snapshots)?;
Review Comment:
Took the second option: the layout is chosen at creation and not afterwards,
in 5d39ab8. Any actual change or removal is rejected whether or not snapshots
exist. What still passes is what moves nothing — a `SetOption` repeating the
stored value, and a `RemoveOption` for an option that is not set — so
reconciliation stays idempotent. That makes Rust stricter than Java on an
option Java itself marks `@Immutable`; the remove-of-an-unset-option case is
the one place it is looser, and it cannot move a file.
Not the lock/CAS: a commit stamps the schema id its writer was built with
and never checks whether the latest schema moved, so ordering schema
publication against snapshot commit would be a commit-path change for every
option, not an index-path one. Worth doing, but not here.
The regression is the sequence you described: a writer resolves the layout,
both conflicting alters are refused, the writer commits, and a freshly loaded
table finds the committed hash index in the bucket data-file directory. Values
are compared literally, by the way — `" TRUE "` parses as `false`, so treating
it as equal to `true` would let a real change through.
Also rebased onto main. #767 split the global index builders and the
scanner, so the routing moved into
`global_index_scanner/{entry,reader,deletion_vectors}.rs`, and #766's FM global
index reader had the same hardcoded `<table>/index` path — it is routed now too.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
