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.

Sign up for GitHub

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
+5 −0
Open

GH-473: Add shredding version #474

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions src/main/thrift/parquet.thrift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -384,6 +384,11 @@ struct BsonType {
* Embedded Variant logical type annotation
*/
struct VariantType {
// If the Variant is shredded the version of the shredding specification used.
// 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 •
edited
Loading

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.

}

/**
Expand Down
Loading