backup: Phase 0b M3b-3 — DynamoDB GSI row derivation (completes M3) - #840
Conversation
Final slice of the DynamoDB reverse encoder (M3): derived global- secondary-index rows. encode_dynamodb_gsi.go is the inverse of the decoder's no-op HandleGSIRow (which DROPS GSI rows precisely because they are derivable from the base item set + schema). It reproduces the live adapter's gsiEntryKeysForItem (V2 ordered path): for each GSI whose key attributes the item carries, emit key = !ddb|gsi|<b64(table)>|<gen>|<b64(index)>| <orderedIndexHash><orderedIndexRange><orderedPKHash><orderedPKRange> value = <the base item's !ddb|item| key> (pointer back to the item) Sparse indexing: an item missing the index hash attribute (or a defined index range attribute) contributes no row for that index — matching the live gsiKeyValues include=false path. Index names are emitted in sorted order (sortedGSIIndexNames parity). The primary-key segment extraction is refactored into ddbOrderedPrimaryKeySegments, shared by the item key and the GSI rows' embedded PK segments so they are guaranteed identical. encodeOneItem now stages the item record followed by its derived GSI rows. Tests (via DecodeLiveEntries, since the decoder drops GSI rows so a directory round-trip can't observe them): a GSI row points back to the item key with the correct !ddb|gsi| prefix; sparse skip for an item lacking the index key; a hand-computed full GSI key-bytes layout assertion; and no-GSI tables emit no rows. Self-review (5 lenses): Data loss — GSI rows derivable, items fully restored, round-trip unaffected; sparse skip matches live. Concurrency — single-goroutine. Performance — O(GSIs) per item, offline. Consistency — key/value reproduce gsiEntryKeysForItem exactly (layout test + value == item key + sparse + sorted names); PK segments shared with the item key; generation = ddbRestoreGeneration throughout. Tests — pointer, sparse, layout, no-GSI. Caller audit: ddbItemKeyBytes refactored to call ddbOrderedPrimaryKeySegments — behavior-preserving (existing key-layout and round-trip tests still pass, byte-identical keys). encodeOneItem gained GSI-row staging after the item record; no existing return semantics changed. ddbGSIRows is new. GSI keys include the full PK so they cannot collide with item keys or each other (b.Add dup-detection would otherwise fail closed).
bootjp
commented
May 26, 2026
@claude review |
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR adds DynamoDB Global Secondary Index (GSI) row derivation to the backup encoder. When encoding items, the system now derives sparse-indexed GSI rows by extracting GSI key segments from each item, computing composite GSI keys with deterministic ordering, and staging both base item records and derived GSI rows into the snapshot. Primary key segment extraction was refactored to support reuse across both item and GSI key construction. ChangesDynamoDB GSI Row Derivation and Integration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Claude finished @bootjp's task in 4m 30s —— View job Code Review — PR #840: DynamoDB GSI Row Derivation
SummaryThe implementation is correct and faithfully reproduces Encoding fidelity — verified ✓
Observation 1 — Missing test: GSI with range key where index range is absent (sparse case 2)
Observation 2 — Multiple GSIs / sorted-order not integration-tested
Observation 3 — Minor inefficiency in |
There was a problem hiding this comment.
Code Review
This pull request introduces Global Secondary Index (GSI) row derivation for DynamoDB backups. It adds encode_dynamodb_gsi.go along with unit tests to derive GSI keys and point them back to base items, and integrates this logic into the main item encoding loop in encode_dynamodb_items.go. The reviewer provided valuable feedback focusing on performance and robustness, including caching sorted GSI names to avoid redundant allocations, pre-allocating slice capacity in ddbGSIIndexKeyPrefix to prevent reallocations, and adding defensive checks for nil schemas to improve error reporting.
| import ( | ||
| "encoding/base64" | ||
| "sort" | ||
| "strconv" | ||
| pb "github.com/bootjp/elastickv/proto" | ||
| ) |
There was a problem hiding this comment.
Add "sync" to the imports to support the thread-safe GSI names cache.
| import ( | |
| "encoding/base64" | |
| "sort" | |
| "strconv" | |
| pb"github.com/bootjp/elastickv/proto" | |
| ) | |
| import ( | |
| "encoding/base64" | |
| "sort" | |
| "strconv" | |
| "sync" | |
| pb"github.com/bootjp/elastickv/proto" | |
| ) |
| func ddbGSIRows(tableName string, generation uint64, schema *pb.DynamoTableSchema, item *pb.DynamoItem, itemKey []byte) ([]ddbGSIRow, error) { | ||
| gsis := schema.GetGlobalSecondaryIndexes() | ||
| if len(gsis) == 0 { | ||
| return nil, nil | ||
| } | ||
| pkHash, pkRange, err := ddbOrderedPrimaryKeySegments(schema, item) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| rows := make([]ddbGSIRow, 0, len(gsis)) | ||
| for _, indexName := range ddbSortedGSINames(gsis) { | ||
| idxHash, idxRange, include, err := ddbGSIKeySegments(item, gsis[indexName].GetKeySchema()) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| if !include { | ||
| continue | ||
| } | ||
| key := ddbGSIIndexKeyPrefix(tableName, generation, indexName) | ||
| key = append(key, idxHash...) | ||
| key = append(key, idxRange...) | ||
| key = append(key, pkHash...) | ||
| key = append(key, pkRange...) | ||
| rows = append(rows, ddbGSIRow{key: key, value: itemKey}) | ||
| } | ||
| return rows, nil | ||
| } | ||
| func ddbSortedGSINames(gsis map[string]*pb.DynamoGlobalSecondaryIndex) []string { | ||
| names := make([]string, 0, len(gsis)) | ||
| for name := range gsis { | ||
| names = append(names, name) | ||
| } | ||
| sort.Strings(names) | ||
| return names | ||
| } |
There was a problem hiding this comment.
Since ddbGSIRows is called for every single item in the table, calling ddbSortedGSINames on every invocation results in redundant slice allocations and sorting of GSI names (which are constant for the entire table). We can cache the sorted GSI names by tableName using a thread-safe cache to completely eliminate this per-item overhead.
var (
gsiNamesCacheMu sync.RWMutexgsiNamesCache=make(map[string][]string)
)
// ddbGSIRows derives the GSI rows for one item. itemKey is the item's// primary !ddb|item| key, stored as each row's value.funcddbGSIRows(tableNamestring, generationuint64, schema*pb.DynamoTableSchema, item*pb.DynamoItem, itemKey []byte) ([]ddbGSIRow, error) {
gsis:=schema.GetGlobalSecondaryIndexes()
iflen(gsis) ==0 {
returnnil, nil
}
pkHash, pkRange, err:=ddbOrderedPrimaryKeySegments(schema, item)
iferr!=nil {
returnnil, err
}
rows:=make([]ddbGSIRow, 0, len(gsis))
for_, indexName:=rangeddbSortedGSINames(tableName, gsis) {
idxHash, idxRange, include, err:=ddbGSIKeySegments(item, gsis[indexName].GetKeySchema())
iferr!=nil {
returnnil, err
}
if!include {
continue
}
extraCap:=len(idxHash) +len(idxRange) +len(pkHash) +len(pkRange)
key:=ddbGSIIndexKeyPrefix(tableName, generation, indexName, extraCap)
key=append(key, idxHash...)
key=append(key, idxRange...)
key=append(key, pkHash...)
key=append(key, pkRange...)
rows=append(rows, ddbGSIRow{key: key, value: itemKey})
}
returnrows, nil
}
funcddbSortedGSINames(tableNamestring, gsismap[string]*pb.DynamoGlobalSecondaryIndex) []string {
gsiNamesCacheMu.RLock()
names, ok:=gsiNamesCache[tableName]
gsiNamesCacheMu.RUnlock()
ifok {
returnnames
}
names=make([]string, 0, len(gsis))
forname:=rangegsis {
names=append(names, name)
}
iflen(names) >1 {
sort.Strings(names)
}
gsiNamesCacheMu.Lock()
gsiNamesCache[tableName] =namesgsiNamesCacheMu.Unlock()
returnnames
}| func ddbGSIIndexKeyPrefix(tableName string, generation uint64, indexName string) []byte { | ||
| encTable := base64.RawURLEncoding.EncodeToString([]byte(tableName)) | ||
| encIndex := base64.RawURLEncoding.EncodeToString([]byte(indexName)) | ||
| gen := strconv.FormatUint(generation, 10) | ||
| out := make([]byte, 0, len(DDBGSIPrefix)+len(encTable)+len(gen)+len(encIndex)+len("|||")) | ||
| out = append(out, DDBGSIPrefix...) | ||
| out = append(out, encTable...) | ||
| out = append(out, '|') | ||
| out = append(out, gen...) | ||
| out = append(out, '|') | ||
| out = append(out, encIndex...) | ||
| out = append(out, '|') | ||
| return out | ||
| } |
There was a problem hiding this comment.
We can avoid multiple slice reallocations when appending key segments to the prefix by pre-allocating the exact capacity needed for the entire key. Passing an extraCap parameter to ddbGSIIndexKeyPrefix allows us to allocate the slice with the exact total capacity in a single step.
| funcddbGSIIndexKeyPrefix(tableNamestring, generationuint64, indexNamestring) []byte { | |
| encTable:=base64.RawURLEncoding.EncodeToString([]byte(tableName)) | |
| encIndex:=base64.RawURLEncoding.EncodeToString([]byte(indexName)) | |
| gen:=strconv.FormatUint(generation, 10) | |
| out:=make([]byte, 0, len(DDBGSIPrefix)+len(encTable)+len(gen)+len(encIndex)+len("|||")) | |
| out=append(out, DDBGSIPrefix...) | |
| out=append(out, encTable...) | |
| out=append(out, '|') | |
| out=append(out, gen...) | |
| out=append(out, '|') | |
| out=append(out, encIndex...) | |
| out=append(out, '|') | |
| returnout | |
| } | |
| funcddbGSIIndexKeyPrefix(tableNamestring, generationuint64, indexNamestring, extraCapint) []byte { | |
| encTable:=base64.RawURLEncoding.EncodeToString([]byte(tableName)) | |
| encIndex:=base64.RawURLEncoding.EncodeToString([]byte(indexName)) | |
| gen:=strconv.FormatUint(generation, 10) | |
| out:=make([]byte, 0, len(DDBGSIPrefix)+len(encTable)+len(gen)+len(encIndex)+len("|||")+extraCap) | |
| out=append(out, DDBGSIPrefix...) | |
| out=append(out, encTable...) | |
| out=append(out, '|') | |
| out=append(out, gen...) | |
| out=append(out, '|') | |
| out=append(out, encIndex...) | |
| out=append(out, '|') | |
| returnout | |
| } |
| func ddbOrderedPrimaryKeySegments(schema *pb.DynamoTableSchema, item *pb.DynamoItem) ([]byte, []byte, error) { | ||
| attrs := item.GetAttributes() | ||
| hashName := schema.GetPrimaryKey().GetHashKey() |
There was a problem hiding this comment.
If schema or schema.GetPrimaryKey() is nil, calling schema.GetPrimaryKey().GetHashKey() is safe due to protobuf's generated getters, but it will return "". This leads to a confusing error message like item missing hash-key attribute "". Adding an explicit defensive check at the beginning of ddbOrderedPrimaryKeySegments will fail closed with a much clearer error message.
funcddbOrderedPrimaryKeySegments(schema*pb.DynamoTableSchema, item*pb.DynamoItem) ([]byte, []byte, error) {
ifschema==nil||schema.GetPrimaryKey() ==nil {
returnnil, nil, errors.Wrap(ErrDDBEncodeInvalidItem, "table schema is missing primary key definition")
}
attrs:=item.GetAttributes()
hashName:=schema.GetPrimaryKey().GetHashKey()…ude #840) Test-only, closing the two new-branch coverage gaps claude flagged (per the CLAUDE.md convention that new branches get unit tests): - TestDDBEncodeGSISparseSkipsMissingIndexRange: a GSI with a defined range key skips an item that has the index hash but not the index range attribute (the second sparse branch in ddbGSIKeySegments). - TestDDBEncodeGSIMultipleIndexes: a two-GSI table emits a row per index (covering ddbSortedGSINames over >1 index), both pointing to the item with their own index prefix. Skipped claude's observations 3 (throwaway hashSeg alloc on the range-skip path) and 4 (nil GSI map entry): both explicitly cosmetic/offline ("doesn't matter" / "low risk") and below the action threshold. No production change.
bootjp
commented
May 26, 2026
@claude review |
Claude finished @bootjp's task in 2m 56s —— View job Code Review — PR #840: DynamoDB GSI Row Derivation
Prior-review gaps: both addressed ✅The initial Claude review (above) flagged two missing test branches. Both were closed in follow-up commit
All five test cases now cover the implementation's decision tree completely. Encoding fidelity — re-verified ✅Cross-checked Both sparse-skip paths in Gemini suggestions — evaluationSuggestion 1 — Thread-safe GSI name cache ( Do not apply. The suggestion adds package-level mutable state (a map that is never evicted) and a mutex to an offline restore tool where sort time over a handful of GSI names is unmeasurable. The added state would also make the package harder to test in parallel. The current simple Suggestion 2 — Pass Minor improvement, but not worth the interface churn. Passing capacity from the caller couples the caller to the callee's internals. The current code does 4 Suggestion 3 — Nil-guard in Worth applying. If which is confusing. The suggested guard: ifschema==nil||schema.GetPrimaryKey() ==nil {
returnnil, nil, errors.Wrap(ErrDDBEncodeInvalidItem, "table schema is missing primary key definition")
}produces a clear, actionable message and is cheap. This is the one gemini suggestion I'd recommend acting on. Fix this → One remaining nit
Refactor quality
Bottom lineImplementation is correct and complete. The two test gaps from the initial review are closed. The only actionable suggestion remaining is the nil-guard for |
bootjp
commented
May 26, 2026
Re: the remaining optional nil-guard on
ifschema.GetPrimaryKey().GetHashKey() =="" {
returnerrors.Wrapf(ErrDDBEncodeInvalidSchema, "%s/_schema.json: empty primary hash key", tableDir)
}
...returne.encodeItems(b, root, tableDir, tableName, schema)So by the time The two test gaps are closed; everything else flagged is cosmetic/offline. No further changes — ready to merge. |
Uh oh!
There was an error while loading. Please reload this page.
Summary
Phase 0b M3b-3 — DynamoDB GSI row derivation. Final slice of the DynamoDB reverse encoder (M3).
encode_dynamodb_gsi.gois the inverse of the decoder's no-opHandleGSIRow(which drops GSI rows precisely because they are derivable from the base item set + schema). It reproduces the livegsiEntryKeysForItem(V2 ordered path):gsiKeyValuesinclude=falsepath. Index names emitted in sorted order.ddbOrderedPrimaryKeySegments, shared by the item key and the GSI rows' embedded PK segments, so they are guaranteed identical.encodeOneItemstages the item record then its GSI rows.Why tests use
DecodeLiveEntriesThe decoder drops GSI rows, so an encode→decode directory round-trip can't observe them. Tests instead encode → write →
DecodeLiveEntriesand inspect the staged!ddb|gsi|records directly.Risk
New file + a behavior-preserving refactor of
ddbItemKeyBytes+ GSI-row staging inencodeOneItem. Offline-tool boundary preserved (GSI key/value format reproduced, not imported).Self-review (5 lenses)
gsiEntryKeysForItemexactly (hand-computed layout test + value == item key + sparse + sorted index names); PK segments shared with the item key; generation =ddbRestoreGenerationthroughout.Caller audit
ddbItemKeyBytesrefactored to callddbOrderedPrimaryKeySegments— behavior-preserving (existing key-layout + round-trip tests pass, byte-identical keys).encodeOneItemgained GSI-row staging after the item record; no existing return semantics changed.ddbGSIRowsis new. GSI keys embed the full PK so they cannot collide with item keys or each other (b.Adddup-detection would otherwise fail closed).Test plan
go test ./internal/backup/(full package)golangci-lint run internal/backup/(0 issues)Summary by CodeRabbit