Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MINOR: Remove Variant type id gaps #483

Merged
merged 1 commit into from
Feb 13, 2025
Merged

Conversation

gene-db
Copy link
Contributor

@gene-db gene-db commented Feb 4, 2025

Rationale for this change

There were some unnecessary gaps in the Variant type id numbering. This removes the id number gaps.

Also, some trivial spacing/formatting changes.

What changes are included in this PR?

Updates to the Variant binary encoding spec.

Do these changes have PoC implementations?

n/a

@gene-db
Copy link
Contributor Author

gene-db commented Feb 4, 2025

@aihuaxu Could you please take a look? I just wanted to make the type ids consecutive and remove the unnecessary gap. Thanks!

@aihuaxu
Copy link
Contributor

aihuaxu commented Feb 4, 2025

@gene-db Initially we had interval types and that's why I leave the gap there.

Thanks for fixing that.

@rdblue
Copy link
Contributor

rdblue commented Feb 12, 2025

Do we need to close this gap? I noticed it as well, but I thought that it was reasonable to reserve these for potential future interval types.

@aihuaxu
Copy link
Contributor

aihuaxu commented Feb 12, 2025

Do we need to close this gap? I noticed it as well, but I thought that it was reasonable to reserve these for potential future interval types.

Yeah. Initially it was there because we had interval types in Spark variant spec. I saw they were removed from Spark spec, assuming they are not getting used at all and Iceberg/Spark will use new typeID if we want to introduce them back. @gene-db and @rdblue

@gene-db
Copy link
Contributor Author

gene-db commented Feb 12, 2025

Yeah, there is no reason for the gap. Spark had the intervals ids momentarily, but they were soon removed, and there were no releases that actually had the interval type ids. Once (or if) we want to add intervals, we can add them with new ids.

@rdblue rdblue merged commit 25f05e7 into apache:master Feb 13, 2025
4 checks passed
@rdblue
Copy link
Contributor

rdblue commented Feb 13, 2025

Spark had the intervals ids momentarily, but they were soon removed, and there were no releases that actually had the interval type ids

Sounds good to me. Thanks!

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.

4 participants