Skip to content

fix(table): skip reserved lineage columns missing from the metrics plan - #1830

Merged
zeroshade merged 1 commit into
apache:mainfrom
ryanworl:pr/metrics-plan-reserved-columns
Aug 14, 2026
Merged

fix(table): skip reserved lineage columns missing from the metrics plan#1830
zeroshade merged 1 commit into
apache:mainfrom
ryanworl:pr/metrics-plan-reserved-columns

Conversation

@ryanworl

Copy link
Copy Markdown
Contributor

What

DataFileStatsFromMeta panics on Parquet files that materialize the reserved v3 row-lineage columns _row_id (field-id 2147483540) and _last_updated_sequence_number (2147483539): writers emit those columns without adding them to the table schema, so they have no entry in the metrics plan, and the zero-value StatisticsCollector panics inside the aggregator on its nil Iceberg type.

After resolving a column's field id, the collector lookup now checks ok. Missing and iceberg.IsMetadataColumn(fieldID): skip the column (no stats). Missing and a real schema column: panic with the field id and column path — a plan/file mismatch is a bug, not a condition to paper over. MetricModeNone still skips as before.

Why

Per the spec (Reserved Field IDs, Row Lineage) these are reserved metadata columns; a writer that materializes them is not required to put them on the table schema. This repo's own position_delta_writer.go does exactly that — it writes _row_id into data files without touching the table schema — so stats collection over such a file panics today.

One disclosed tightening: a nil or partial StatsCols plan now panics deterministically for any real schema column holding values (previously it could pass silently when the column happened to contain only nulls). TestWriteDataFileErrOnClose was updated to supply a plan covering the list-element leaf so it reaches the close error it actually tests.

Tests

  • Reserved _row_id outside the plan is skipped and produces no stats; a non-reserved schema column outside the plan panics with the field id and path.
  • go test ./table/... and golangci-lint run are clean.

Made with Cursor

DataFileStatsFromMeta panics on Parquet files that materialize the
reserved v3 row-lineage columns (_row_id / field-id 2147483540,
_last_updated_sequence_number / field-id 2147483539). Writers write
those columns without adding them to the table schema or the metrics
plan (Iceberg spec, Reserved Field IDs). Skip only iceberg.IsMetadataColumn
ids; any other field id missing from the plan is a plan/file mismatch
and still panics, rather than aggregating through a zero-value collector.

Co-authored-by: Cursor <cursoragent@cursor.com>
@ryanworl
ryanworl requested a review from zeroshade as a code owner August 13, 2026 20:18

@zeroshade zeroshade left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I verified the bug is real and the fix is in the right place. MetricModeType's zero value is "" (not "none", table/internal/utils.go:365-371), so a file column missing from the plan sailed past the MetricModeNone check and panicked in createStatsAgg on the nil IcebergTyp as soon as the column carried min/max stats. The paths that actually hit this are the ones that recompute stats from an existing file with a plan built from the table schema — fileToDataFile (arrow_utils.go:1768, add_files) and DataFileMetaToDataFile (data_file_meta.go:292) — since the compat check tolerates extra provided columns and the file-derived colMapping resolves _row_id to 2147483540. The !ok + IsMetadataColumn skip is the right shape, and panicking for a non-reserved miss matches the existing convention in this function (the colMapping panic just above) with both external callers already wrapping in recover.

One small inaccuracy in the description: the position_delta_writer.go write path itself doesn't panic today — WithPreserveRowLineage threads the lineage-augmented fileSchema into the writer factory, and computeStatsPlan runs on f.fileSchema after the options are applied (rolling_data_writer.go:209), so _row_id is in the plan there. The panic bites when a lineage-materializing file's stats are recomputed later (add_files / DataFileMeta), which the fix correctly covers. Might be worth tweaking the commit message.

On the disclosed tightening: the practical case it affects is add_files over an old file that still materializes a since-dropped column — that already panicked when the column held values (nil-type createStatsAgg), and now also fails deterministically when it's all-null instead of silently recording counts under a field ID no longer in the schema. Both call sites recover the panic into an error, so this is a behavior improvement, not a crash risk.

Tests are adequate (skip + loud-failure both covered), the TestWriteDataFileErrOnClose plan update is a correct consequence, and go test ./table/... is green on the PR head.

@zeroshade
zeroshade merged commit 02c94ed into apache:main Aug 14, 2026
15 checks passed
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.

2 participants