Conversation
mythical-fred
left a comment
There was a problem hiding this comment.
Clean scope: one-commit fork bump + a real regression test. The fork's v0.32.3-feldera-fix branch is rust-v0.32.3 + the two existing patches + the one new patch 297f2dd1 (comparison is 3-ahead / 0-behind of upstream, 1-ahead / 0-behind of the previously-pinned 5b35c098), so nothing is silently rebased or dropped. The Cargo.toml comment tracks all three patches, points at the new compare URL, and the lockfile update matches.
The regression test earns its keep: after is a six-field nested struct whose children are themselves column-mapped to diverging col-<id> names, so it exercises field-id resolution inside the struct rather than just at the top level — that is the "no field name overlap" struct-cast failure mode from #6557, not only the col-102 is missing case. op VARCHAR NOT NULL also covers the hard-error branch, and the fixture is built by pyarrow (via ensure_delta_spark_fixture), which is the only way to get logical-name Parquet on disk. FIXTURE_VERSION is present for cache invalidation. Good hygiene.
Nits, non-blocking:
- Consider marking the test with the same
@pytest.mark.used by other delta-input suites so it participates in the same selectors — I only see the baredef test_.... If the rest oftest_delta_input_*.pysimilarly has no marker, ignore. - Comment nit:
# (logical child name, field id) for the six-field after struct.— withAFTER_CHILDREN[0][1] == 104, the top-level ids areid=100/after=101/op=102and children start at104(skipping103). Not wrong, just worth a one-line note in the fixture header so a future reader doesn't assume 103 is missing by accident. - Changelog checkbox is unchecked in the PR body; a one-line "delta connector: read UC-Uniform-over-Iceberg column-mapped tables by Parquet field id (#6557)" under Unreleased would be nice for release notes but isn't strictly needed for a bug fix.
Approve. Solid, minimal, tests the actual failure surface, and the fork diff is auditable.
| # Feldera fork of delta-rs: upstream tag `rust-v0.32.3` + 2 patches, on branch | ||
| # `v0.32.3-feldera`. When bumping the pinned revision, preserve these patches: | ||
| # Feldera fork of delta-rs: upstream tag `rust-v0.32.3` + 3 patches, on branch | ||
| # `v0.32.3-feldera-fix`. When bumping the pinned revision, preserve these patches: |
There was a problem hiding this comment.
I hope we have submitted these to upstream and that someone is looking at them.
There was a problem hiding this comment.
not yet, but will do when i get time
23acfe9 to
7b7ca2d
Compare
Bump the delta-rs fork to 9a58fbef, which resolves columnMapping.mode=id columns by Parquet field-id rather than physical name. Without it, reading a UC-Uniform-over-Iceberg table fails with `col-<id> is missing from the physical schema`. Add a snapshot-read regression test (fixture + e2e). Signed-off-by: Swanand Mulay <73115739+swanandx@users.noreply.github.com>
|
QA passed, merging this |
mythical-fred
left a comment
There was a problem hiding this comment.
Re-review at tip ee4f35aa.
Additive delta since my last APPROVE (23acfe97):
- Fork rev bumped
5b35c098 -> 297f2dd1on branchv0.32.3-feldera-fix, adding a third patch9a58fbef"delta-rs: resolve column-mapped Uniform/Iceberg columns by Parquet field-id". Fixescol-<id> is missing from the physical schemawhen reading UC-Uniform-over-Iceberg tables wherecolumnMapping.mode=id. Cargo.toml header updated to list all three patches with the correct compare URL; matches Gerd's convention for the fork-pinning block. - New pyarrow-driven fixture
python/tests/platform/fixtures/uniform_iceberg.py(217 lines) that hand-writes the Parquet +_delta_login the exact shape a native Iceberg writer + Unity Catalog Uniform produces: Parquet columns named by their logical names carryingPARQUET:field_id, Delta log usingcolumnMapping.mode='id'with divergentcol-<N>physical names — at both top level AND inside theafterstruct's six children. Delta Spark can't produce this shape, so the pyarrow route is correct.store_schema=Falseis right to keep the file Iceberg-shaped. - New
python/tests/platform/test_delta_input_uniform_iceberg.py(139 lines) exercising the customer'scdc_rawfailure on two fronts:- Non-nullable top-level
op(physicalcol-102) — pre-fix errors with "Non-nullable column 'col-102' is missing from the physical schema". - Nested
afterstruct whose six children are each mapped tocol-<id>— pre-fix fails the struct cast ("Cannot cast struct with 6 fields to 6 fields because there is no field name overlap"). The test projects a nested child ofafter, so field-id resolution is exercised at every level, not just the top.
FIXTURE_VERSION = "v1"bump comment covers the caching gotcha.
- Non-nullable top-level
QA per swanandx's comment above. Re-approving.
One small ask (non-blocking, per mihaibudiu's L115 comment on my last pass): please do submit the three patches upstream when you get cycles — the fork's diverging longer than we'd like.
Bump the delta-rs fork with fix which resolves columnMapping.mode=id columns by Parquet field-id rather than physical name. Without it, reading a UC-Uniform-over-Iceberg table fails with
col-<id> is missing from the physical schema. Add a snapshot-read regression test (fixture + e2e).Fix #6557
Describe Manual Test Plan
created custom runtime, and verified the fix works
Checklist
Breaking Changes?
should not be breaking change