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

GH-473: Add shredding version #474

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

emkornfield
Copy link
Contributor

@emkornfield emkornfield commented Dec 4, 2024

Rationale for this change

Shredding of variants has a lot of potential evolutions having a version helps track any future version. Note the binary format is already versioned because of this.

What changes are included in this PR?

Do these changes have PoC implementations?

Variant is still work in progress.

Closes #473

@emkornfield
Copy link
Contributor Author

CC @rdblue @gene-db

@emkornfield emkornfield changed the title GH-472: Add shredding version GH-473: Add shredding version Dec 4, 2024
// Required, if the the column is shredded.
//
// See VariantShredding.md for differences between versions.
1: optional i8 shredding_version
Copy link
Contributor

Choose a reason for hiding this comment

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

What about encoding version? If we include the encoding version, that would help us change it in the future. For shredding, do we want to have a single version that includes shredding or a separate version for shredding?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

encoding version is already embedded in metadata, so I thought wouldn't be needed here. Shredding version is only encoded implicitly based on the spec without this.

I think shredding should probably evolve differently from encoding (hopefully we won't need to evolve any for some time). But shredding seems like we need at least some more experimentation to determine if there are other forms that might be better.

I might not be fully understanding the question here though.

Copy link
Contributor

Choose a reason for hiding this comment

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

Would that mean that we allow mixed versions based on the metadata for each record? That seems like unnecessary complication to me. And it also means that older clients would fail at read time when they encounter a newer record, rather than failing quickly at the schema check stage.

I think it makes sense to put the encoding version here and expect uniform encoding throughout a Parquet file. Writers should produce the latest encoding, not carry records through.

For the shredding question, I would rather have one version of variant instead of evolving them separately. I think that would get confusing and there would be dependencies between them. For example, can you shred a type defined by a newer version of the encoding? Probably not, so bumping the encoding version also requires bumping the shredding version. So is it worth it to have one version number that can increase independently? I would combine them into a single variant version for shredding and encoding.

Copy link
Contributor Author

@emkornfield emkornfield Dec 12, 2024

Choose a reason for hiding this comment

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

Would that mean that we allow mixed versions based on the metadata for each record? That seems like unnecessary complication to me. And it also means that older clients would fail at read time when they encounter a newer record, rather than failing quickly at the schema check stage.

In discussions on footer optimization most of the community seemed to lean towards not putting in extra metadata to allow for quick failure. Putting this value in the schema I think also disallows simple merging of non-shredded values between two different versions (and as a strong indication that the variant is shredded, I think it is an interesting API consideration on whether we separate API for interrogating shredded columns or return the schema "as is"). The reason I want to version the shredding is to ensure we aren't reliant on detecting column name differences to determine version but maybe even this is premature.

I think it makes sense to put the encoding version here and expect uniform encoding throughout a Parquet file. Writers should produce the latest encoding, not carry records through.

We can add it but I think it is just as reasonable require if there is ever a V2, that all values within a row group are consistent which I think gives uniformity at a reasonable level?

For example, can you shred a type defined by a newer version of the encoding? Probably not, so bumping the encoding version also requires bumping the shredding version. So is it worth it to have one version number that can increase independently? I would combine them into a single variant version for shredding and encoding.

Right, but I don't think the inverse is true, it is likely we can have different shredding versions based on the same binary encoded format which is why I think versioning them separately makes sense.

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.

Add shredding version to Variant logical annotation
2 participants