Skip to content

fix(fast-path): use manifest max_field_id for new fields#46

Merged
universalmind303 merged 1 commit into
daft-engine:mainfrom
FANNG1:fix/fast-path-next-fid-child-ids
Jul 5, 2026
Merged

fix(fast-path): use manifest max_field_id for new fields#46
universalmind303 merged 1 commit into
daft-engine:mainfrom
FANNG1:fix/fast-path-next-fid-child-ids

Conversation

@FANNG1

@FANNG1 FANNG1 commented Jun 26, 2026

Copy link
Copy Markdown
Contributor

Fixes #41

Problem

FastPathFragmentWriter computed the field ID for new columns from top-level schema fields:

next_fid = max(f.id() for f in self.lance_ds.lance_schema.fields()) + 1

lance_schema.fields() does not provide a complete high-water mark for field IDs. Nested fields, such as struct children, have their own Lance field IDs. Schema evolution can also leave previously used field IDs behind. If next_fid is based only on visible top-level fields, the new data file can be committed with a field ID that collides with an existing nested or historical field ID.

Example for a table with id (fid=0) and a struct meta (fid=1) with children meta.a (fid=2) and meta.b (fid=3):

Source max fid next_fid Effect
Top-level fields only 1 2 Can collide with meta.a
Dataset max_field_id 3 4 Uses the manifest high-water mark

With the wrong next_fid, fragment metadata can associate the new file with the wrong schema field. Reads of the intended new column can then return null or wrong values.

Fix

Use Lance's manifest-level field ID high-water mark:

next_fid = self.lance_ds.max_field_id + 1

max_field_id reflects nested child fields and field IDs reserved by prior schema evolution, so it is the correct source for assigning new field IDs.

Test

Adds TestRegressions::test_next_fid_uses_manifest_max_field_id, which creates a dataset with a struct column, merges a new scalar column through the fast path, and verifies that the new column reads back with the expected values.

@FANNG1 FANNG1 force-pushed the fix/fast-path-next-fid-child-ids branch 3 times, most recently from 9233c54 to a160516 Compare July 3, 2026 01:32
@FANNG1 FANNG1 changed the title fix(fast-path): recurse into child fields when computing next_fid fix(fast-path): use manifest max_field_id for new fields Jul 3, 2026
@FANNG1 FANNG1 force-pushed the fix/fast-path-next-fid-child-ids branch 3 times, most recently from 0c5852b to ecbe266 Compare July 4, 2026 00:11
lance_schema.fields() returns only top-level fields. Nested types such as structs have child fields with their own IDs, so max(f.id() for top-level f) can choose a next_fid that collides with an existing child field ID.

Use Lance's manifest-level max_field_id as the high-water mark for new column field IDs. This also accounts for nested child fields and field IDs from prior schema evolution.

Fixes daft-engine#41
@FANNG1 FANNG1 force-pushed the fix/fast-path-next-fid-child-ids branch from ecbe266 to b164df9 Compare July 5, 2026 00:40
@universalmind303 universalmind303 merged commit 577fe61 into daft-engine:main Jul 5, 2026
5 checks passed
@FANNG1 FANNG1 deleted the fix/fast-path-next-fid-child-ids branch July 6, 2026 01:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug(fast-path): next_fid collides with nested struct child field IDs, causing new columns to read back as null

2 participants