connectors: fix delta connector issues with unity catalog in follow mode - #6746
Conversation
mythical-fred
left a comment
There was a problem hiding this comment.
LGTM. Commits are clean and the "why" is well captured; unit tests cover the field-id realign and nested relabel paths, and the Python fixture exercises the follow path against a real mode=name table with a nested struct. Deferring to @mihaibudiu's inline notes.
0cbe9cc to
5948317
Compare
mythical-fred
left a comment
There was a problem hiding this comment.
Re-review of the seven new commits on top of my 2026-07-27 approval. Mihai's inline feedback is addressed cleanly: the doc for field_to_physical is trimmed and no longer describes sibling functions, relabel_array_data's doc no longer over-explains, and the deeper-nested tests he asked for are in — structs in structs, arrays in structs, structs in arrays, structs in arrays of structs, plus a struct-of-struct-of-array-of-struct end-to-end check.
The new work on top is substantive and warranted:
uc://follow now routes through the object-store reader unconditionally (empty DV bitmap when there's no real deletion vector) instead of aListingTable— a path-less UC URL couldn't resolveroot_url() + Add.path, so the previous path read empty.table_root_baseguarantees the trailing slash for other schemes so the same join stays well-formed.- Field-id-based column resolution (
realign_array+project_to_logicalindeletion_vector.rs) coverscolumnMapping.mode=idtables whose files name columns logically but stampPARQUET:field_id, and where the read schema usescol-<id>names withdelta.columnMapping.id. Struct children match by id first, position only as a fallback for unmapped, and missing children null-fill — matching the top-level behavior. - Container-type coercion (
ListvsLargeList) viacastfor non-struct types is a real gap the earlier code would have hit as a hard error.
Tests are thorough: field-id resolution, missing-child null-fill, list-container coercion, and the nested-relabel round trip preserving buffers. Commit history is clean — no AI-attribution trailers.
Approving.
Signed-off-by: Swanand Mulay <73115739+swanandx@users.noreply.github.com>
…und) root_url() for a uc:// table has no trailing slash, so joining Add.path corrupted the object-store authority. Normalize the root before joining. Signed-off-by: Swanand Mulay <73115739+swanandx@users.noreply.github.com>
The follow/CDC read builds its own Parquet ListingTable and renamed
column-mapped fields to their physical (col-<uuid>) names only at the top
level. Nested struct children kept their logical names, so reading a
mode=name table with a nested struct failed the Parquet struct cast
("Cannot cast struct ... no field name overlap") and dropped the rows,
while a snapshot read (which delta-rs resolves) worked.
Recurse the physical read schema into nested struct fields, and restore
their logical names per batch in execute_df_inner (top-level names are
still restored on the DataFrame by project_physical_to_logical). The
per-batch relabel is keyed on a nested-only map, so flat and
mapping-off tables are untouched.
Signed-off-by: Swanand Mulay <73115739+swanandx@users.noreply.github.com>
A uc:// location is path-less, so the URL-addressed ListingTable built from root_url() + Add.path reads empty and silently drops the commit. Route uc:// follow reads (and DV files) through the delta-rs object store directly, the reader snapshot and CDC-masked reads already use. Other schemes keep the ListingTable path unchanged. Signed-off-by: Swanand Mulay <73115739+swanandx@users.noreply.github.com>
columnMapping.mode=id tables (UniForm over Iceberg) write data files with logical column names plus a Parquet field id, while the Delta log assigns diverging col-<id> physical names. The follow reader matched columns by name, so it could not find col-<id> in the file, null-filled, and dropped the data (or failed on a non-nullable column). Match file columns to the read schema by field id (PARQUET:field_id on the file, delta.columnMapping.id on the schema), rebuilding nested struct/list/map children and casting leaves. Falls back to name when a field id is absent, so mode=name and unmapped tables are unaffected. Mirrors the snapshot path's field-id resolution. Signed-off-by: Swanand Mulay <73115739+swanandx@users.noreply.github.com>
realign_array recursed into list/largelist/map and downcast to the exact container the read schema named, so a file whose list kind differed (Delta List vs a file's LargeList) failed with an internal error. Rebuild only structs by field id (where column-mapped names must line up) and route everything else, including container coercion, through cast. Signed-off-by: Swanand Mulay <73115739+swanandx@users.noreply.github.com>
realign_array fell back to the positional child when a target struct child's field id had no match in the file. That could bind an unrelated sibling column, or, when the position was out of range, fail with an internal error. A column-mapped struct that gains a nested child through schema evolution hits both. Match nested children by field id only, and null-fill a child the file lacks, mirroring how project_to_logical handles a missing top-level column. Positional matching now applies solely to the unmapped case, where neither side carries a field id. Signed-off-by: Swanand Mulay <73115739+swanandx@users.noreply.github.com>
A failed conversion surfaced Arrow's bare cast error, which names neither the column nor the file: "Casting from Utf8 to FixedSizeBinary(16) not supported". Under column mapping the column is named col-<uuid> on disk, so the user had no way to tell which column of which table failed. Thread the file and a dotted column path through realign_array, so a nested leaf reports 'after.col-2' rather than the top-level column, and report the file's type alongside the type the Delta table declares. Signed-off-by: Swanand Mulay <73115739+swanandx@users.noreply.github.com>
e69fab1 to
3029b6d
Compare
Note: CDC-over-uc:// is not covered by this PR. The plain-file CDC read path (cdc_side_dataframe) still uses the URL-addressed ListingTable, which reads empty over a path-less uc:// location, the same way follow did before this fix.
Describe Manual Test Plan
tested out in staging
Checklist
Breaking Changes?
no