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

API, Core: Add formatVersion() to Table #11587

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

nastra
Copy link
Contributor

@nastra nastra commented Nov 19, 2024

No description provided.

Copy link
Contributor

@munendrasn munendrasn left a comment

Choose a reason for hiding this comment

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

lgtm

@@ -212,6 +212,11 @@ public String toString() {
return name();
}

@Override
public int formatVersion() {
return table().formatVersion();
Copy link
Member

Choose a reason for hiding this comment

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

Do metadata tables have format versions? I would argue no?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I don't think it makes sense for metadata tables to have a format version. The entries in the tables could be spread across different "versions" of a table where the underlying table had different format versions.

Copy link
Contributor Author

@nastra nastra Nov 20, 2024

Choose a reason for hiding this comment

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

so the only table where this would be currently necessary to have is PositionDeletesTable. This is so that we can know in SparkPositionDeletesRewrite (which runs against PositionDeletesTable) whether existing position deletes need to be rewritten to either V2 positional deletes or to DVs

Copy link
Member

@RussellSpitzer RussellSpitzer left a comment

Choose a reason for hiding this comment

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

LGTM, minor nits

Copy link
Contributor

@amogh-jahagirdar amogh-jahagirdar left a comment

Choose a reason for hiding this comment

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

I think the API makes a lot of sense, it'll shrink all the awkward call points which cast to HasTableOperations just to get the operations. The only doubt I have is if it makes sense for metadata tables to have this API. I don't think it makes complete sense since a metadata table could have entries which span different format versions for the underlying table

@nastra
Copy link
Contributor Author

nastra commented Nov 21, 2024

I've created an alternative impl to this in #11620 where we can achieve the same thing without adjusting the Table API.

@aokolnychyi
Copy link
Contributor

I think I'd prefer #11620 for now, not a strong opinion, though.

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.

6 participants