Skip to content

Reject conflicted source-id and source-ids fields - #3645

Open
ebyhr wants to merge 1 commit into
apache:mainfrom
ebyhr:ebi/source-ids
Open

Reject conflicted source-id and source-ids fields#3645
ebyhr wants to merge 1 commit into
apache:mainfrom
ebyhr:ebi/source-ids

Conversation

@ebyhr

@ebyhr ebyhr commented Jul 10, 2026

Copy link
Copy Markdown
Member

Rationale for this change

The original validator only ran multi-argument handling when source-id was absent (if "source-id" not in data and "source-ids" in data). When both keys were present, source-ids was silently ignored.

Are these changes tested?

Yes

Are there any user-facing changes?

No.

@github-actions

Copy link
Copy Markdown

This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that's incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions.

@github-actions github-actions Bot added the stale label Aug 10, 2026

@rambleraptor rambleraptor left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I've got one small style nit, but otherwise this looks great.

def map_source_ids_onto_source_id(cls, data: Any) -> Any:
if isinstance(data, dict):
if "source-id" not in data and "source-ids" in data:
if "source-ids" in data:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggested change
if "source-ids" in data:
if "source-ids" in data and "source-id" in data:
raise ValueError("source-id and source-ids are mutually exclusive")
elif "source-ids" in data:

style nit. Nested if statements can confusing to read.

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