Reconcile virtual_column_exec with upstream row_id (DESIGN required first)

[zfarrell]

Context

This is one ticket in a series carrying forward #12 foundation work. Read #12 first for repo context.

This is the highest-risk ticket in the set because it requires a design reconciliation, not just a port. The fork added src/virtual_column_exec.rs (5 virtual columns: rowid, snapshot_id, file_index, filename, file_row_number). Independently, upstream landed src/row_id.rs in PR datafusion-contrib#115, providing a single rowid virtual column via a different mechanism (an embedded _ducklake_internal_row_id field-id protocol).

We need one cohesive design, not two. The fork's surface is a superset, but upstream's design choice (embedded field-id) is more elegant. The right answer probably involves keeping the upstream mechanism and extending it to cover the remaining four columns.

Reference branches

• Fork: ducklake-features/integration — src/virtual_column_exec.rs, tests/virtual_column_tests.rs (6 tests, 7-column schema assertions), tests/virtual_column_extended_tests.rs (13 tests, multi-file catalogs, contiguity assertions)
• Upstream: datafusion-contrib/datafusion-ducklake:main — src/row_id.rs, PR feat: DuckLake row lineage (rowid virtual column) datafusion-contrib/datafusion-ducklake#115 discussion

Scope

1. Read both implementations end-to-end. Spend time on this — do not implement before understanding the design intent of each.
2. Read the DuckLake spec section on virtual columns to confirm which columns are spec-mandated vs nice-to-have.
3. Propose a unified design in a comment on this issue before implementing. The likely shape:
   • Keep upstream's embedded-field-id mechanism for rowid (it interoperates with the DuckLake spec's row-lineage convention)
   • Add snapshot_id, file_index, filename, file_row_number as additional virtual columns under the same mechanism
   • Or, if the mechanisms genuinely cannot unify, document why and pick one cleanly
4. Implement the agreed design. Replace both src/virtual_column_exec.rs and the supersedable parts of src/row_id.rs.
5. Port over the fork's tests (virtual_column_tests.rs, virtual_column_extended_tests.rs) — adjust assertions to match the chosen mechanism, but preserve the behavioral coverage (multi-file rowid contiguity, WHERE-clause filtering on virtual columns, NULL handling when row_id_start/snapshot_id is missing in the catalog).

Acceptance criteria

• Design proposal posted as a comment and approved before implementation begins
• All 5 virtual columns work via a single mechanism
• Existing upstream rowid semantics preserved (no regression for downstream consumers)
• tests/virtual_column_tests.rs and tests/virtual_column_extended_tests.rs adapted and passing
• WHERE-clause pushdown: SELECT * FROM t WHERE rowid = N does the right thing (filter pushdown is correct, no full scan when avoidable)
• Single-partition coercion preserved when file_row_number or rowid is requested (fork uses CoalescePartitionsExec for this)
• Checked arithmetic for rowid overflow preserved
• No duckdb crate imports

Dependencies

• Depends on: Foundation: rebase integration onto upstream, drop pass-throughs, triage SLT failures #12 (foundation)

Out of scope

• New virtual columns beyond the 5-column union — _change_type for CDC views is a separate ticket
• Writeable virtual columns — not a thing in DuckLake

Notes

• Audit verdict on the fork's virtual_column_exec.rs: "solid — correctly forces single partition when row-number-dependent columns are requested via CoalescePartitionsExec, uses checked arithmetic for rowid overflow, handles missing row_id_start/snapshot_id as null."
• The reconciliation matters because wrong choice here means downstream consumers either break or have to migrate twice. This is the one ticket where "going slower" is the right answer.
• If reconciliation reveals upstream's embedded-field-id design has a fundamental gap (e.g., can't express filename without breaking the embedding), that's evidence the upstream design needs to evolve — raise it as an upstream issue rather than silently working around it.

[zfarrell]

Design decision (autonomous; documented in lieu of pre-approval)

Chosen path: Option C (hybrid) — embedded rowid + synthesized scan-time metadata, unified behind a single with_row_lineage opt-in.

Mechanism split

Column	Mechanism	Why
rowid	Upstream's embedded-field-id model (src/row_id.rs) — read _ducklake_internal_row_id (parquet field-id 2_147_483_540) for UPDATE/compaction rewrites; RowIdExec cursor (row_id_start + position_in_file) for INSERT-only files	Spec-required ("rowid survives file rewrites"). The fork's pure cursor approach is mathematically incapable of producing the correct values after an UPDATE rewrites the file — verified by rowid_preserved_across_update_rewrite. Cross-engine compatibility with DuckDB hinges on this.
filename, file_row_number, snapshot_id, file_index	Synthesized at scan time via VirtualColumnExec	All four are scan-time metadata, not row-level data. There's nothing for them to "survive" — they describe where a row was read from, not anything stored in the row. Embedding them would be invasive (extra columns per row) and awkward (filename is per-file metadata; file_row_number is inherently scan-time).

Visibility model

with_row_lineage(bool) (default false) gates all five virtual columns as a single bundle, not just rowid. Rationale:

• The row_id_tests pre-existing in this fork (from upstream PR feat: DuckLake row lineage (rowid virtual column) datafusion-contrib/datafusion-ducklake#115) hard-assert that SELECT * FROM t with row lineage disabled returns only the physical schema and that SELECT rowid errors out — incompatible with the fork's previous "5 cols always visible" surface.
• DuckDB's spec hides rowid by default too. We don't have a hidden-column concept in DataFusion, but a single opt-in flag is the closest analog.
• Treating the four scan-time columns as a different visibility bundle from rowid would create three distinct surfaces (off / rowid-only / all-five) with no obvious user model. One flag, one mode.

Per-file scan dispatch

When row lineage is on and a query touches any virtual column, the scan path now goes:

1. For each data file, detect whether the parquet embeds _ducklake_internal_row_id (field-id 2147483540). Cached per file in a Mutex<HashMap<path, Option<String>>>, mirroring upstream PR feat: DuckLake row lineage (rowid virtual column) datafusion-contrib/datafusion-ducklake#115's FileReadConfig cache.
2. If rowid is in projection:
   • Embedded file → extend the parquet read_schema with the rowid column, project it alongside real cols, apply delete filtering, rename it to rowid via ColumnRenameExec.
   • Cursor file → real-column scan → RowIdExec(row_id_start) (appends rowid in file order) → DeleteFilterExec (elides deleted rowids alongside their rows; critical ordering — rowid_preserved_under_deletes regressed when I had it the other way around).
3. If any of the four scan-time columns are in projection → wrap the rowid-bearing exec (or raw scan when rowid isn't requested) with VirtualColumnExec, which appends filename / file_row_number / snapshot_id / file_index.
4. Output column order at the per-file level: [real_proj..., rowid?, filename?, file_row_number?, snapshot_id?, file_index?]. A trailing ProjectionExec reorders when DataFusion's requested projection isn't in that canonical order.

RowIdExec and VirtualColumnExec both required a fix to propagate the child's EquivalenceProperties.oeq_class onto their appended-schema output — without it, a SortPreservingMergeExec over a UnionExec of per-file streams (which is what ORDER BY id produces) fails DataFusion's SanityCheckPlan ("does not satisfy order requirements"). The wrappers don't reorder, only append, so child orderings remain valid; we just need to re-attach them.

Branch

zfarrell/feat/22-virtual-columns (branched from upstream-port/integration, 4 commits).

Acceptance checklist

• All 5 virtual columns (rowid, snapshot_id, file_index, filename, file_row_number) work via the chosen mechanisms
• Existing upstream rowid semantics preserved — tests/row_id_tests.rs (6/6 pass after the TODO(#22) markers were resolved, including rowid_preserved_across_update_rewrite which exercises the embedded-field-id path)
• tests/virtual_column_tests.rs (6/6) and tests/virtual_column_extended_tests.rs (13/13) pass with the new surface — they now enable with_row_lineage(true) and assert the same behavioral coverage (multi-file rowid contiguity, WHERE pushdown on rowid/snapshot_id/file_index, NULL handling)
• WHERE-clause pushdown for rowid filters — test_where_on_rowid, test_where_on_file_index, test_where_on_snapshot_id
• Single-partition input enforced when row-number-dependent columns are requested — required_input_distribution() == SinglePartition on both RowIdExec and VirtualColumnExec
• Checked arithmetic for rowid overflow preserved (in RowIdExec; the embedded-read path can't overflow — it reads the value directly)
• No duckdb crate imports added
• TODO(#22) markers all resolved (only doc references to ticket Reconcile virtual_column_exec with upstream row_id (DESIGN required first) #22 remain)
• with_row_lineage is no longer a stub — flag flows catalog → schema → table; toggle materially changes scan behavior

Commit log

50c42a2 test(virtual-columns): align test surface with row-lineage opt-in
8f5f4bb feat(virtual-columns): hybrid rowid + scan-time metadata design
7368430 fix(row-id): propagate child orderings on appended schema
b765d3d refactor(virtual-columns): drop rowid from VirtualColumnExec

Test delta vs post-#21 baseline (1,428 passing, 140 failing — DuckDB Parquet-footer flakes)

Suite	Before	After
row_id_tests	0/6 (all ignored under TODO(#22))	6/6 passing
virtual_column_tests	6/6 (asserting 5-col rowid-included shape)	6/6 (asserting same shape, now via with_row_lineage(true) setup)
virtual_column_extended_tests	13/13	13/13
write_tests, sqlite_metadata_provider_test, mysql_/postgres_metadata_provider_test	1 test in each was hard-coding "real + 5 virtual" column counts	now expect "real only" (the row-lineage-disabled default); 4 tests updated to match the new opt-in semantics
cross_engine_feature_tests virtual-column suite (4 tests)	depended on always-on virtual cols; would have broken under the new surface	catalog factory now sets with_row_lineage(true); tests pass

Full-suite totals: baseline 1,428–1,429 / 139–140 → after #22: 1,433–1,435 / 139. Net +6 passing (the previously-ignored row_id_tests), no net new failures. All cross-engine DuckDB Parquet-footer-length failures are pre-existing on upstream-port/integration and unrelated to this ticket.

Interaction with other tickets

• ALTER/DROP/CREATE schema evolution DDL #20 (ALTER TABLE): alter_table_tests (24/24) and cross_engine_alter_tests (13 passing, 3 failing pre-existing Parquet-footer flakes) continue to pass. ALTER doesn't interact with the virtual-column surface — virtual columns are not part of the catalog's column metadata, so they can't be renamed / dropped / added by user DDL.
• CDC table functions (with cdc_common duplicate-column bug fix) #21 (CDC functions): table_changes_tests (9/9 passing, 4 ignored pre-existing) untouched.