fix(schema): enforce executable type evolution - #681
Conversation
ALTER TYPE admitted Arrow-castable pairs without guaranteeing Paimon-compatible reads of historical files. Predicate pruning and global indexes could also retain stale physical-type semantics. Add explicit evolution executors and history preflight checks, apply safe residual filtering and writer normalization, and fence schema-incompatible global index entries across catalog and DataFusion paths. Signed-off-by: QuakeWang <wangfuzheng0814@foxmail.com>
| {"name": "_INDEX_META", "type": ["null", "bytes"], "default": null}, | ||
| {"name": "_SOURCE_META", "type": ["null", "bytes"], "default": null} | ||
| {"name": "_SOURCE_META", "type": ["null", "bytes"], "default": null}, | ||
| {"name": "_BUILD_SCHEMA_ID", "type": ["null", "long"], "default": null} |
There was a problem hiding this comment.
It records the table schema ID used to build a global-index entry. It closes a race with ALTER COLUMN TYPE: schema evolution does not advance the data snapshot, so an index built with the old field type may still pass the latest-snapshot commit guard. Readers compare the indexed field types from this schema with the current schema and ignore incompatible entries. Legacy and Java entries leave it null and are handled conservatively through schema history.
There was a problem hiding this comment.
After reviewing the mixed-version write path, I prefer option (1) for this PR unless coordinated Java support is shipped first. Moving the field to a trailing top-level position lets the old Rust decoder skip it, but an old Java IndexManifestFileHandler still deserializes entries into its six-field model and rewrites the whole combined manifest, so it drops either a nested or top-level structured field. That loses provenance and leaves new Rust unable to use or rebuild the index safely after evolution. A future compatible design should either gate writers on a Java version that preserves the field or place the schema ID in an opaque payload old writers round-trip unchanged.
JingsongLi
left a comment
JingsongLi
left a comment
There was a problem hiding this comment.
The addition of _BUILD_SCHEMA_ID breaks compatibility with older Rust readers. Testing has confirmed that the new writer produces two Index Manifests, causing the current main reader to report unknown FileKind: 7.
|
@JingsongLi Thanks, confirmed. The nested |
| let result: ArrayRef = match timestamp.precision() { | ||
| 0..=3 => Arc::new( | ||
| downcast_primitive::<TimestampMillisecondType>(array, "TIMESTAMP")? | ||
| .unary::<_, Date32Type>(|value| (value / 86_400_000) as i32), |
There was a problem hiding this comment.
[P1] Use floor division for pre-epoch timestamps
Rust integer division truncates toward zero, but Paimon Java TimestampToDateCastRule uses Math.floorDiv. Consequently -1 ms becomes epoch day 0 instead of -1, and -86_400_001 ms becomes -1 instead of -2, silently shifting pre-1970 non-midnight values and residual-filter results by one day. Please use div_euclid in the millisecond, microsecond, and nanosecond branches and change the new test expectation to [-1, -1, -2, null]; the current test passes only because it codifies the wrong truncation.
| table.schema(), | ||
| &new_schema, | ||
| ) | ||
| .await?; |
There was a problem hiding this comment.
[P1] Fence this preflight with the schema version
The validation is based on a prior get_table, while AlterTableRequest carries only changes and no expected schema ID. For example, this client can validate INT -> BIGINT, a concurrent Java client can commit INT -> DECIMAL and write a Decimal file, and then this stale POST is applied by the server as DECIMAL -> BIGINT (supported by Java). The final history now requires DECIMAL -> BIGINT, which this PR's Rust executor does not implement, so Rust fails to read the intermediate file despite the preflight succeeding. The preflight must run atomically on the server, or the request must assert the schema ID and retry validation on conflict.
| } | ||
| } | ||
| Some((col, source_type)) => columns.push( | ||
| cast_array_for_schema_evolution( |
There was a problem hiding this comment.
[P1] Keep in-flight readers able to read post-ALTER files
This replacement of the generic Arrow cast assumes the table schema is always newer than the file schema, but long-lived providers keep a fixed Table while their scans plan the latest snapshot. A provider registered under INT can therefore see a new DECIMAL file after another client performs INT -> DECIMAL; here the source is Decimal and the target is the provider's Int schema, and the new one-way executor rejects DECIMAL -> INT even though the previous Arrow cast handled it. This breaks running readers/jobs across ALTER. Please either implement the reverse executors required by old-schema readers or fence/refresh providers so a fixed schema cannot scan newer file schemas.
| .apply_changes(changes) | ||
| .map_err(|e| fill_table_name(e, identifier))?; | ||
| let new_schema = apply_schema_changes(¤t, &changes, identifier)?; | ||
| validate_type_evolution_precommit(&self.file_io, &table_path, ¤t, &new_schema) |
There was a problem hiding this comment.
[P1] Include the schema version in the index-build commit guard
This preflight and schema save are not coordinated with global-index commits, and an ALTER does not advance the snapshot ID. An index build planned at snapshot S/schema N can therefore commit after this saves schema N+1 because commit_if_latest_snapshot still sees S; the commit stamps the index with N and also creates a new snapshot whose schema_id is the stale N. Fresh Rust drops the index, time travel/branch creation observes the wrong schema, and an old Java reader has no build-schema filter and may use the stale index. Please atomically guard index commits on both snapshot and schema ID (or serialize them with ALTER), and stamp snapshots from the latest schema as Java does.
| {"name": "_INDEX_META", "type": ["null", "bytes"], "default": null}, | ||
| {"name": "_SOURCE_META", "type": ["null", "bytes"], "default": null} | ||
| {"name": "_SOURCE_META", "type": ["null", "bytes"], "default": null}, | ||
| {"name": "_BUILD_SCHEMA_ID", "type": ["null", "long"], "default": null} |
There was a problem hiding this comment.
[P2] Do not let old Java rewrites erase this marker
Current Java GlobalIndexMeta has six fields, and IndexManifestFileHandler reads all active entries and serializes a new combined manifest. An old Java writer therefore drops this seventh field on any index/DV manifest rewrite. Back on new Rust, a valid index built after type evolution is treated as legacy/incompatible and filtered out, while final overlap validation still retains the physical entry and blocks rebuilding it until an explicit drop. A trailing top-level field fixes the old-Rust decode issue but is still lost by an old serializer. Please gate emission on a Java version that preserves the marker, or encode the provenance in an opaque payload old writers round-trip unchanged.
2 participants
Purpose
ALTER COLUMN TYPEpreviously accepted Arrow-castable type pairs without guaranteeing Paimon-compatible conversion of historical files. Predicates, statistics, and global indexes could therefore use incompatible physical-type semantics.Brief change log
Tests
API and Format
Adds an optional
build_schema_idto global-index metadata. Legacy index entries remain readable.Documentation
Documents the supported type-evolution matrix and safety restrictions.