Skip to content

source-ids is not supported on Iceberg v1,2#2114

Closed
rambleraptor wants to merge 5 commits intoapache:mainfrom
rambleraptor:source-ids
Closed

source-ids is not supported on Iceberg v1,2#2114
rambleraptor wants to merge 5 commits intoapache:mainfrom
rambleraptor:source-ids

Conversation

@rambleraptor
Copy link
Copy Markdown
Contributor

Closes #1547

Rationale for this change

The field source-ids is being introduced for Iceberg v3. According to the spec, it should not be supported by Iceberg v1 + v2. This introduces an error message to ensure that it cannot be used in those versions.

Most of the code here is to plumb through the format_version to the field to do the actual version check.

Are these changes tested?

Unit tests included.

Are there any user-facing changes?

This does change the current support for source-ids on Iceberg v1/v2, but it shouldn't be supported in the first place.

Copy link
Copy Markdown
Contributor

@Fokko Fokko left a comment

Choose a reason for hiding this comment

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

Hey @rambleraptor thanks for working on this, but I don't think this is the right approach. This breaks a lot of current APIs, and also requires users to pass in versions when creating PartitionFields and SortOrderFields.

A while ago, I've did some work on the spec to avoid having to carry through the table version to all the parsers, to exactly avoid this situation :) I think we should be good with just parsing either source-id or source-ids.

Comment thread pyiceberg/partitioning.py
@rambleraptor
Copy link
Copy Markdown
Contributor Author

That makes a lot of sense for v3. But, it looks like source-ids is supported on v1/v2, which the spec explicitly disallows. We'd have to carry through the table version to fix that. If that doesn't seem like a serious issue, happy to close this PR.

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.

Deprecate source-id in favor of source-ids

2 participants