Skip to content

feat: read v3 row lineage metadata columns#822

Open
wgtmac wants to merge 3 commits into
apache:mainfrom
wgtmac:read_row_lineage
Open

feat: read v3 row lineage metadata columns#822
wgtmac wants to merge 3 commits into
apache:mainfrom
wgtmac:read_row_lineage

Conversation

@wgtmac

@wgtmac wgtmac commented Jul 6, 2026

Copy link
Copy Markdown
Member

Synthesize _row_id and _last_updated_sequence_number when reading data files that omit row lineage columns, and preserve physical non-null lineage values when present.

Carry inherited data sequence numbers on DataFile metadata so scan planning and readers follow the same model as Java.

auto options =
MakeReaderOptions(*data_file, io_, projected_schema_, task.residual_filter(),
name_mapping_, properties_);
auto options = MakeReaderOptions(

@manuzhang manuzhang Jul 7, 2026

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.

REST scan-task JSON appears to round-trip only first-row-id for DataFile; FileScanTasksFromJson constructs the FileScanTask without any data sequence number. That means this call can pass data_file->first_row_id but an empty data_file->data_sequence_number, and MakeLastUpdatedSequenceNumberArray will return nulls for _last_updated_sequence_number.

The row-lineage spec says missing/null _last_updated_sequence_number should be replaced with the data file’s data sequence number.

Disclosure: I used an LLM to help review this PR and draft this comment.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I looked into the Java REST implementation and OpenAPI. This is not C++-specific: RESTFileScanTaskParser serializes the task file through ContentFileParser, and ContentFileParser only round-trips first-row-id, not the manifest-entry data sequence number. The OpenAPI FileScanTask/DataFile schema also has first-row-id but no field for the data sequence number.

So the failing case is REST-planned scan tasks reading row lineage metadata, where _last_updated_sequence_number needs the manifest entry sequence number but that value is not part of the REST contract. I think the correct fix should start in the REST API / Java parser, then C++ can implement the same field. I don’t think C++ should add a non-standard private field here.

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.

Agreed, the fix is beyond this PR. We can create a follow-up issue to track.

Comment thread src/iceberg/avro/avro_data_util.cc Outdated
Comment thread src/iceberg/inheritable_metadata.cc
wgtmac added 2 commits July 11, 2026 15:47
Synthesize _row_id and _last_updated_sequence_number when reading
data files that omit row lineage columns, and preserve physical
non-null lineage values when present.

Carry inherited data sequence numbers on DataFile metadata so scan
planning and readers follow the same model as Java.
@wgtmac wgtmac force-pushed the read_row_lineage branch from 410a4f5 to 46b9737 Compare July 11, 2026 08:15
Result<std::shared_ptr<::arrow::Array>> ProjectRowLineageArray(
const std::shared_ptr<::arrow::Array>& physical_array, int32_t field_id,
const arrow::MetadataColumnContext& metadata_context, ::arrow::MemoryPool* pool) {
if (!arrow::HasRowLineageValue(field_id, metadata_context)) {

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.

It looks physical array is not examined and its non-null value might not be preserved with first_row_id present but data_sequence_number absent. Both Avro generic decoding and direct decoding have similar issues.

@WZhuo WZhuo left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

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.

3 participants