Skip to content

Conversation

@rdblue
Copy link
Contributor

@rdblue rdblue commented Apr 18, 2025

This adds readers for _row_id and _last_updated_sequence_number in Spark and the generic data model.

@rdblue rdblue requested a review from amogh-jahagirdar April 18, 2025 00:04
@rdblue rdblue force-pushed the row-lineage-readers branch from 35b8714 to 4f984c5 Compare April 18, 2025 17:51
@github-actions github-actions bot removed the API label Apr 18, 2025
@rdblue rdblue changed the title Spark: Add _row_id reader Spark: Add _row_id and _last_updated_sequence_number readers Apr 18, 2025
ParquetValueReader<?> reader =
ParquetValueReaders.replaceWithMetadataReader(
id, readersById.get(id), idToConstant, constantDefinitionLevel);
reorderedFields.add(defaultReader(field, reader, constantDefinitionLevel));
Copy link
Contributor Author

@rdblue rdblue Apr 18, 2025

Choose a reason for hiding this comment

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

The changes here were needed to reduce the complexity of the struct method that was failing checkstyle. I refactored into 2 components:

  • ParquetValueReaders.replaceWithMetadataReader handles metadata columns and constants
  • defaultReader handles defaults based on the schema; this requires convertConstant so it is not shared in ParquetValueReaders

I also simplified the constant handling that was introduced in #4627. That PR introduced a map with the max definition level for columns that are replaced with constants and defaults the definition level to the parent struct's value when there is no underlying column (for instance, _pos). This was over-complicated because the fields don't need a column-specific DL. If the parent struct is non-null then the constant should be added to it, so the DL can always be the parent's DL.

public static ParquetValueReader<Long> lastUpdated(
Long baseRowId, Long fileLastUpdated, ParquetValueReader<?> seqReader) {
if (fileLastUpdated != null && baseRowId != null) {
return new LastUpdatedSeqReader(fileLastUpdated, (ParquetValueReader<Long>) seqReader);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

baseRowId is used to detect non-v3 snapshots. It is required for v3 snapshots so when it is missing, the values should be null. That's required by the spec update in #12781.

@rdblue rdblue force-pushed the row-lineage-readers branch from 0dc3d17 to a18b51f Compare April 18, 2025 21:20
}

ConstantReader(C constantValue, int definitionLevel) {
ConstantReader(C constantValue, int parentDl) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: I'd probably use the full form "parentDefinitionLevel"

@amogh-jahagirdar amogh-jahagirdar merged commit ab92d6e into apache:main Apr 21, 2025
42 checks passed
@amogh-jahagirdar
Copy link
Contributor

Thanks @rdblue !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants