vinishjail97 commented on PR #861: URL: https://github.com/apache/incubator-xtable/pull/861#issuecomment-5258675696
@AadhiKat thanks for the round-2 work, and sorry for the slow turnaround on my side. The profiling write-ups made this much easier to review. Rather than send another round of comments, I've pushed `cd928ea1` to the branch directly (maintainer edits were enabled) with the remaining items from my last review. Happy to drop or amend it if you'd rather take them yourself, just say so. One substantive change beyond what I'd asked for, which I want to flag rather than bury: **the config now defaults to `false`.** This is a core path used in production by everyone syncing from Delta, and the optimization only pays off on the tiny-file plus long-history shape from #860. I'd rather ship it opt-in for a release, let the people hitting the pathology turn it on, and revisit the default once it has mileage. Your numbers make the case for the optimization itself; my hesitation is only about who gets it silently. The rest, thread by thread: - `resetState` now clears `cachedMetadata`, so the invariant is per backlog rather than per source instance, plus a `Preconditions` guard on non-decreasing versions. - Config moved to `DeltaConversionSourceConfig` with `fromProperties`, mirroring `DeltaConversionTargetConfig`, and a unit test covering the parsing. - The flag now reverts all three behaviour changes, not just the snapshot reuse. With it off, `getChangeLogFiles` never runs and `latestCommitTime` comes from `Snapshot.timestamp()` exactly as on main. - `DeltaTableExtractor.table(Snapshot, ...)` delegates to the `Metadata` overload. - Dropped the duplicate deletion-vector null check. - Corrected the stale rationale comment in the two-run test. `mvn spotless:check` is clean, xtable-core unit tests are 494 passing, and `ITDeltaConversionSource` is 19 passing. On your two open questions from the last round: leaving `DeltaIncrementalChangesState` with the two listings is fine by me, the single pass isn't worth reimplementing Delta's action parsing for a cost that measures inside noise. And yes please, a separate issue for the same treatment on `DeltaKernelConversionSource` would be great, this PR is big enough. -- 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]
