Core: Extend V4 DeletionVector with key_metadata field - #17438
Core: Extend V4 DeletionVector with key_metadata field#17438gaborkaszab wants to merge 1 commit into
Conversation
| "Number of set bits (deleted rows) in the vector"); | ||
| Types.NestedField KEY_METADATA = | ||
| Types.NestedField.optional( | ||
| 149, |
There was a problem hiding this comment.
ColumnFiles PR takes field IDs 158-169. I see 149 is free
| } | ||
|
|
||
| @Override | ||
| public boolean equals(Object other) { |
There was a problem hiding this comment.
I haven't added key_metadata here. I think we should rather drop equals() and hashCode. We agreed such metadata structs shouldn't override these. Will remove them in a follow-up PR.
anoopj
left a comment
anoopj
left a comment
There was a problem hiding this comment.
Can we also update the spec PR @amogh-jahagirdar is working on and the AMT proposal doc?
| long cardinality(); | ||
|
|
||
| /** Returns encryption key metadata, or null if the file is not encrypted. */ | ||
| ByteBuffer keyMetadata(); |
There was a problem hiding this comment.
I think we need to update the adapter TrackedDVDeleteFile to return the key metadata? (right now it just hard returns null).
There was a problem hiding this comment.
I initially figured I just take care of the schema and structure changes and deal with the rest on top later. Since the change in the adapter is minimal, I covered it too as you suggested. Thanks, @anoopj !
95fb136 to
908a23a
Compare
gaborkaszab
left a comment
gaborkaszab
left a comment
There was a problem hiding this comment.
Thanks for taking a look, @anoopj !
@amogh-jahagirdar would you mind checking this to be in sync with the spec changes you have in mind?
| long cardinality(); | ||
|
|
||
| /** Returns encryption key metadata, or null if the file is not encrypted. */ | ||
| ByteBuffer keyMetadata(); |
There was a problem hiding this comment.
I initially figured I just take care of the schema and structure changes and deal with the rest on top later. Since the change in the adapter is minimal, I covered it too as you suggested. Thanks, @anoopj !
Deletion vectors need their own encryption key metadata. Adds field 149 (key_metadata, optional binary) to the deletion_vector struct, matching apache#17438.
| default: | ||
| throw new UnsupportedOperationException("Unknown field ordinal: " + pos); | ||
| } | ||
| return switch (pos) { |
There was a problem hiding this comment.
I remember some discussion around using the newer Java language switch statement but I don't know where it landed, are we good with this style now? cc @nastra @RussellSpitzer @stevenzwu
There was a problem hiding this comment.
I'm not opinionated on this one, I just want to make sure the style is consistent
amogh-jahagirdar
left a comment
amogh-jahagirdar
left a comment
There was a problem hiding this comment.
Overall the change looks great to me, I updated the spec PR as well to include this. Just a question on the switch style.
3 participants
No description provided.