Re: [PR] [886] Add Delta Kernel source/target coverage to ITConversionController [incubator-xtable]

["vaibhavk1992 (via GitHub)" <gi...@apache.org>]

vaibhavk1992 commented on PR #903:
URL: https://github.com/apache/incubator-xtable/pull/903#issuecomment-5971676477

   @vinishjail97 Rebased onto latest main and addressed all three review comments:
   - Truncation safety test now asserts real outcomes instead of just logging (incl. the allMatch-on-empty-backlog fix).
   - Kernel target matrix now covers unchanged-schema upserts and deletes before the schema-evolution assertions.
   
   CI should be green on the latest push — let me know if anything needs another pass.
   
   Separately, closing out the full prerequisite 2/3 re-verification from #886:
   
   #711, #813, #641 — already covered earlier: all three dispositioned as not Kernel-specific (root causes are in IcebergSchemaSync.addUpdates, Hudi's own filesystem listing, and a pre-Delta Iceberg-source crash respectively). No Kernel action needed.
   
   #779 — already covered earlier: ITDeltaLogTruncationSafetyCheck confirms Kernel fails safely (throws KernelException) rather than reading stale/incomplete data, both in the straightforward and TOCTOU-race scenarios.
   
   #586 (Iceberg→Delta metadata reconciliation) — found and verified end-to-end (using the actual production code, not just inspection) a path-normalization inconsistency: DeltaDataFileUpdatesExtractor/DeltaConversionTarget compute the table base path via Path.toString(), while DeltaActionsConverter.tableBasePath(Snapshot) uses Path.toUri().toString(). These diverge for base paths containing characters that need percent-encoding, and I reproduced the exact reported symptom (spurious add+remove for an unchanged file) by running the real getRelativePath/getFullPathToFile round-trip with a divergent path. Checked whether the Kernel flip regresses this — it doesn't: Kernel's Table.getPath(engine) just echoes back the exact string passed to Table.forPath, so there's no second normalization path to diverge from. Green for the flip (Kernel doesn't reproduce it); the Standalone-side inconsistency itself is still open and belongs to #586, not this PR.
   
   slachiewicz's column-mapping findings (physicalName/columnMapping.mode) — confirmed both are real: DeltaKernelSchemaExtractor.getFieldMetadata only writes delta.columnMapping.id, never physicalName, and the Kernel target never sets delta.columnMapping.mode. But traced Kernel's actual validation path (ColumnMapping.updateColumnMappingMetadataIfNeeded) — it short-circuits to a no-op when mode is NONE, which is always the case today, so the asymmetric metadata is currently unused/unreachable. Verified empirically with an ICEBERG→Kernel-target sync, which completes cleanly. Green for the flip as well, with a note that it's a dormant gap, not a fixed one — worth flagging so nobody enables column mapping on a Kernel target later without knowing.
   
   #860 — Kernel's getTableChangeForCommit has the identical unoptimized pattern (fresh getSnapshotAsOfVersion per commit, no caching) that Standalone had before the #860 fix. Since Standalone's fix defaults its reuse flag to false, Kernel matches Standalone's default behavior — not a regression. The only gap: Kernel has no equivalent to the opt-in reuse_metadata_across_commits flag, so anyone relying on that today would lose it on Kernel. Green for the flip (no default-path regression); worth a tracking issue if large-table incremental syncs on Kernel become painful, but not a blocker.
   
   Deletion vectors (#886 cites #713 for this, but that's actually the original Kernel-integration issue, not deletion vectors — the real open item is #347, about writing DVs to a target, unimplemented on both engines, not Kernel-specific, fine to just document as a known limitation) — found something new while checking this, unrelated to #347: InternalDataFile.recordCount is wrong on both Standalone and Kernel when a source file has an active deletion vector — neither converter subtracts deletionVector.cardinality from the file's raw stats.numRecords. Verified against the identical on-disk Delta log: actual live rows = 40, Standalone reports 50, Kernel reports 50 (identically wrong). ITDeltaDeleteVectorConvert never caught this because its only assertion compares file paths, never row counts. This predates the Kernel work entirely — not a flip blocker, but want to file it as its own issue rather than fold it into #886. Let me know if you'd like me to open that, or if you wan
 t to take a first look.
   
   Summary for #886 prerequisite 2/3: everything checked comes back green for the flip itself (Kernel doesn't regress any of these) — the remaining open items (#586's Standalone fix, the deletion-vector recordCount bug, #860's missing Kernel-side caching) are all pre-existing or newly-found gaps that apply independently of Kernel, not things the flip makes worse.
   
   Given all of the above, if you're good with this, I'll follow up with the actual default flip — flipping xtable.delta.target.use_kernel to true, keeping it as an escape hatch for one release, and calling it out in the release notes per #886's "the change itself" section. Let me know and I'll get that PR started.


-- 
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: commits-unsubscribe@xtable.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org