[886] Add Delta Kernel source/target coverage to ITConversionController - #903
[886] Add Delta Kernel source/target coverage to ITConversionController#903vaibhavk1992 wants to merge 5 commits into
Conversation
- testVariousOperationsDeltaKernelSource runs the same dataset-equivalence checks as testVariousOperations, forcing DeltaKernelConversionSourceProvider across both sync modes and both partitioning cases. - testVariousOperationsDeltaKernelTarget does the same for the DELTA target leg, routing through DeltaKernelConversionTarget via DeltaConversionTargetConfig.USE_KERNEL, using ConversionTargetFactory's normal dispatch path. - testVariousOperations body extracted into runVariousOperationsTest so both new tests share the same assertions the Standalone paths are validated with; existing Standalone matrix is unchanged. The target-side test currently fails: DeltaKernelConversionTarget only calls withSchema() on table creation, so schema evolution on an existing table (e.g. a new nested field added inside an array column) never reaches the Delta log's Metadata action. Traced to open upstream gap delta-io/delta#4305. Follow-up fail-fast guard tracked separately.
DeltaKernelConversionTarget.commitTransaction() only applies withSchema() on CREATE_TABLE (Delta Kernel 4.0.0 limitation, delta-io/delta#4305), so schema evolution on an already-existing table was silently dropped: rows kept syncing, but a newly-added nested field (e.g. inside a struct that is an array column's element type) never reached the Delta log's registered schema, making it unreadable even though the underlying Parquet had it. syncSchema() now compares the incoming schema (as a Kernel StructType, to avoid any InternalSchema<->StructType round-trip noise) against the existing snapshot's schema, and throws NotSupportedException as soon as they differ for an existing table, instead of committing a stale schema. ConversionController/TableFormatSync catches this per-target and records it as a SyncStatusCode.ERROR, leaving the target at its last-good state rather than partially written. testVariousOperationsDeltaKernelTarget updated accordingly: it no longer reuses runVariousOperationsTest's full lifecycle (which assumes success), since the exception surfaces as a SyncResult status rather than a thrown exception from ConversionController.sync(). It now syncs an initial snapshot (expected SUCCESS), evolves the schema, syncs again (expected ERROR with the delta-io/delta#4305 reference), and confirms the target's row count didn't move.
testCreateSnapshotControlFlow and testTimestampNtz each did two syncSnapshot calls to the same existing table, with the second call's schema silently adding a field on top of the first (schema2 = schema1 + float_field). This is exactly the schema-evolution-on-an-existing-table scenario DeltaKernelConversionTarget now rejects (see the syncSchema guard added earlier). Before that guard existed, Kernel silently accepted the second sync without registering the new schema, and these tests only checked physical file presence/size, not schema correctness, so they never caught that the second sync's schema change was being dropped. Neither test is actually about schema evolution: testCreateSnapshotControlFlow is about the file add/remove control flow across two sequential snapshot syncs, and testTimestampNtz is about TIMESTAMP_NTZ handling. Fixed both to reuse the same schema for their second snapshot, matching their real intent. Added testSchemaEvolutionOnExistingTableFailsSync as a dedicated test for the guarded behavior: first sync succeeds, second sync (with an evolved schema) returns SyncStatusCode.ERROR referencing delta-io/delta#4305, and the table is left at its last-good state rather than partially written.
85aef09 to
58683ce
Compare
slachiewicz
left a comment
slachiewicz
left a comment
There was a problem hiding this comment.
The coverage matches prerequisite 1 of #886: source and target, both sync modes and both partitioning cases. Ran the two new tests locally on JDK 11, ITConversionController#testVariousOperationsDeltaKernelSource+testVariousOperationsDeltaKernelTarget → 8/8 pass.
The only red check is spotless: two comment wraps in DeltaKernelConversionTarget plus the assertThrows and NotSupportedException imports that 58683ce left unused. ./mvnw spotless:apply fixes both, but run it on JDK 17 or older, since google-java-format 1.10.0 refuses newer JVMs. Everything else in that run was green, ITConversionController included.
One gap worth closing: nothing syncs an existing table twice with an unchanged schema through the Kernel target, which is the path the new guard can false-positive on. syncSchema compares cachedSnapshot.getSchema() against schemaExtractor.fromInternalSchema(...), so any asymmetry in the InternalSchema/StructType round trip turns a no-op sync into a SyncStatusCode.ERROR. TestDeltaKernelReadWriteIntegration#testIncrementalUpdates covers this for a simple schema only; a second unchanged-schema sync asserting SUCCESS in testVariousOperationsDeltaKernelTarget's first block would extend it to the partitioned and timestamp cases.
Scope note for #886: this lands prerequisite 1 and the fail-fast, not the default flip. While the target fails fast, making Kernel the default turns every schema-evolving Delta pipeline into an error, so the flip still waits on either the fallback to Standalone or delta-io/delta#4305.
This comment was created with AI assistance.
|
Thanks for the review @slachiewicz Spotless: fixed in 07cf223. Why throw instead of falling back to Standalone: DeltaConversionTarget.init() If you need schema evolution today: set xtable.delta.target.use_kernel=false to route |
…eltaKernelTarget Per PR apache#903 review feedback: nothing exercised syncing an already-existing table twice with an unchanged schema through the Kernel target, which is exactly the path the syncSchema() drift guard could false-positive on if there's any asymmetry in the InternalSchema<->StructType round trip. Added a second sync (50 more rows, same schema) to the first block of testVariousOperationsDeltaKernelTarget, asserting SyncStatusCode.SUCCESS, before moving on to the schema-evolution block that expects ERROR. Covers both partitioned and unpartitioned cases via the existing parameterization.
|
@vinishjail97 did some validation for prerequisite 2. |
2 participants
The target-side test currently fails: DeltaKernelConversionTarget only calls withSchema() on table creation, so schema evolution on an existing table (e.g. a new nested field added inside an array column) never reaches the Delta log's Metadata action. Traced to open upstream gap delta-io/delta#4305. Follow-up fail-fast guard tracked separately.
Important Read
What is the purpose of the pull request
(For example: This pull request implements the sync for delta format.)
Brief change log
(for example:)
Verify this pull request
(Please pick either of the following options)
This pull request is a trivial rework / code cleanup without any test coverage.
(or)
This pull request is already covered by existing tests, such as (please describe tests).
(or)
This change added tests and can be verified as follows:
(example:)