feat(#492): implement SetStatistics, RemoveStatistics, AddEncryptionKey, RemoveEncryptionKey updates#902
Conversation
|
@zeroshade Can you please review this change and merge if the changes are okay? |
| type EncryptionKey struct { | ||
| KeyID string `json:"key-id"` | ||
| KeyMetadata *string `json:"key-metadata,omitempty"` | ||
| KeyAlgorithm *string `json:"key-algorithm,omitempty"` | ||
| } |
There was a problem hiding this comment.
These aren't the correct json keys according to the spec, for example it should be encrypted-key-metadata not key-metadata, there is no KeyAlgorithm in the spec and you're missing the "Properties" member. Please update to follow the v3 spec
There was a problem hiding this comment.
Fixed. Renamed to encrypted-key-metadata, removed key-algorithm, added properties field per the V3 spec.
| SnapshotRefs map[string]SnapshotRef `json:"refs,omitempty"` | ||
| StatisticsList []StatisticsFile `json:"statistics,omitempty"` | ||
| PartitionStatsList []PartitionStatisticsFile `json:"partition-statistics,omitempty"` | ||
| EncryptionKeyMap map[string]EncryptionKey `json:"encryption-keys,omitempty"` |
There was a problem hiding this comment.
According to the spec, encryption-keys is a list, not a map
There was a problem hiding this comment.
Right, I've changed from map[string]EncryptionKey to []EncryptionKey throughout metadata, builder, and interface.
laskoviymishka
left a comment
There was a problem hiding this comment.
maps.Equal on EncryptionKey is broken — *string fields compared by pointer address, not value. V3 gating missing for encryption keys.
| slices.Equal(c.SnapshotLog, other.SnapshotLog) && slices.Equal(c.MetadataLog, other.MetadataLog) && | ||
| iceinternal.SliceEqualHelper(c.SortOrderList, other.SortOrderList) | ||
| iceinternal.SliceEqualHelper(c.SortOrderList, other.SortOrderList) && | ||
| maps.Equal(c.EncryptionKeyMap, other.EncryptionKeyMap) |
There was a problem hiding this comment.
i think this is a bug
EncryptionKey has *string fields (KeyMetadata, KeyAlgorithm). maps.Equal uses == — compares pointer addresses, not values. Two keys deserialized from the same JSON will have different pointers. Equals() returns false for identical metadata. Use maps.EqualFunc with reflect.DeepEqual like SnapshotRefs three lines above.
There was a problem hiding this comment.
Now using iceinternal.SliceEqualHelper with an Equals() method on EncryptionKey that does proper value comparison of *string fields and map[string]string properties.
| } | ||
|
|
||
| // AddEncryptionKey adds or replaces an encryption key indexed by its key-id. | ||
| func (b *MetadataBuilder) AddEncryptionKey(key EncryptionKey) error { |
There was a problem hiding this comment.
Encryption keys are V3+. No format version check. The codebase already gates V3 features — see minFormatVersionRowLineage. Add a b.formatVersion < 3 guard.
There was a problem hiding this comment.
Fixed. AddEncryptionKey now rejects calls when formatVersion < 3 with a descriptive error, consistent with the existing minFormatVersionRowLineage pattern.
…Key, RemoveEncryptionKey updates
Adds the four remaining table update types required for REST catalog
compatibility. Without these, any commit response containing these
actions fails with "unknown update action".
Changes:
- table/encryption.go: new file; defines EncryptionKey type (key-id,
key-metadata, key-algorithm) separate from statistics types
- table/statistics.go: remove EncryptionKey (moved to encryption.go)
- table/updates.go: add constants and update structs for
set-statistics, remove-statistics, add-encryption-key,
remove-encryption-key; register all four in UnmarshalJSON
- table/metadata.go:
- add EncryptionKeys() iter.Seq2[string, EncryptionKey] to Metadata
interface and implement on commonMetadata
- add EncryptionKeyMap field to commonMetadata with JSON tag
encryption-keys (omitempty; inert on V1/V2 tables)
- add encryptionKeyMap to MetadataBuilder; initialize in
NewMetadataBuilder alongside refs; copy in MetadataBuilderFromBase;
write through in buildCommonMetadata
- add SetStatistics (upsert by snapshot-id), RemoveStatistics,
AddEncryptionKey, RemoveEncryptionKey builder methods
- include EncryptionKeyMap in commonMetadata.Equals
- table/updates_test.go: unmarshal and Apply tests for all four
update types including replace-not-append semantics for
SetStatistics and no-op remove cases
- Fix EncryptionKey struct: rename key-metadata to encrypted-key-metadata, remove key-algorithm, add encrypted-by-id and properties fields - Change encryption-keys from map to list per spec - Fix equality check using Equals method instead of maps.Equal - Add V3 format version guard to AddEncryptionKey
a9efcfd to
98adfba
Compare
Adds the four remaining table update types required for REST catalog compatibility. Without these, any commit response containing these actions fails with "unknown update action".
Changes:
Feature #492